[PATCH] D56066: [OpenCL] Address space for default class members

ebevhan added inline comments. Comment at: lib/Sema/SemaInit.cpp:4539 + if (InitCategory.isPRValue() || InitCategory.isXValue()) +T1Quals.removeAddressSpace(); + rjmccall wrote: > rjmccall wrote: > > I can understand why a pr-value wouldn't have an address s

[PATCH] D55868: [Fixed Point Arithmetic] Fixed Point Addition Constant Expression Evaluation

ebevhan added inline comments. Comment at: clang/include/clang/Basic/FixedPoint.h:98 public: + APFixedPoint() = default; APFixedPoint(const llvm::APInt &Val, const FixedPointSemantics &Sema) rjmccall wrote: > This should be documented to describe what it

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

ebevhan added a comment. Thanks, that would be great! Hopefully it will work this time. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51211/new/ https://reviews.llvm.org/D51211 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

ebevhan updated this revision to Diff 178605. ebevhan added a comment. Use `auto`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51211/new/ https://reviews.llvm.org/D51211 Files: lib/Sema/SemaChecking.cpp test/Sema/format-strings-bitfield-promotion.c test/Sema/format-strings-bitf

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

ebevhan marked an inline comment as done. ebevhan added inline comments. Comment at: lib/Sema/SemaChecking.cpp:7725-7726 +return true; + // Look through vector types, since we do default argument promotion for + // those in OpenCL. + if (const ExtVectorType *VecTy = From->

[PATCH] D55656: [OpenCL] Address space for default class members

ebevhan added inline comments. Comment at: lib/AST/ASTContext.cpp:2781 + + return getAddrSpaceQualType(NewT, Orig.getAddressSpace()); } rjmccall wrote: > Anastasia wrote: > > rjmccall wrote: > > > You're trying to handle a method qualifier, not a type a functio

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

ebevhan added a comment. I'm also a bit confused about the semantics that this patch is applying to function types. It mostly seems to concern the extra trailing Qualifiers on CXXMethodDecl to store the addrspace quals, but in some places (SemaType:4842, SemaDecl:3198) it seems to be applying t

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

ebevhan added inline comments. Comment at: lib/CodeGen/CGCall.cpp:77 + if (MD) +RecTy = Context.getAddrSpaceQualType(RecTy, MD->getType().getAddressSpace()); return Context.getPointerType(CanQualType::CreateUnsafe(RecTy)); I'm a bit late to the party, bu

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

ebevhan updated this revision to Diff 177831. ebevhan added a comment. Fix the build failures (caused by default argument promotion of float vectors). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51211/new/ https://reviews.llvm.org/D51211 Files: lib/Sema/SemaChecking.cpp test/Sema

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

ebevhan added a comment. Hmm, sorry about that. I'll have a look. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51211/new/ https://reviews.llvm.org/D51211 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

ebevhan added a comment. Thanks! I don't have commit access, so I would appreciate it if you could commit the change. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51211/new/ https://reviews.llvm.org/D51211 ___ cfe-commits mailing list cf

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

ebevhan added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:1304 +RHSTy = ResultTy; + } + rjmccall wrote: > ebevhan wrote: > > rjmccall wrote: > > > leonardchan wrote: > > > > rjmccall wrote: > > > > > leonardchan wrote: > > > > > > rjmccall wrote

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

ebevhan added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:1304 +RHSTy = ResultTy; + } + rjmccall wrote: > leonardchan wrote: > > rjmccall wrote: > > > leonardchan wrote: > > > > rjmccall wrote: > > > > > leonardchan wrote: > > > > > > rjmccall w

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

ebevhan updated this revision to Diff 176571. ebevhan added a comment. Added testing for C++ and different sizes of `int` and `long`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51211/new/ https://reviews.llvm.org/D51211 Files: lib/Sema/SemaChecking.cpp test/Sema/format-strings-b

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

ebevhan added inline comments. Comment at: test/Sema/format-strings-bitfield-promotion.c:1 +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s + aaron.ballman wrote: > ebevhan wrote: > > ebevhan wrote: > > > aaron.ballman wrote: > > > > aa

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

ebevhan marked an inline comment as done. ebevhan added inline comments. Comment at: test/Sema/format-strings-bitfield-promotion.c:1 +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s + ebevhan wrote: > aaron.ballman wrote: > > aaron.ball

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

ebevhan marked 2 inline comments as done. ebevhan added inline comments. Comment at: test/Sema/format-strings-bitfield-promotion.c:1 +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s + aaron.ballman wrote: > aaron.ballman wrote: > > Runn

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

ebevhan added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:1304 +RHSTy = ResultTy; + } + rjmccall wrote: > leonardchan wrote: > > rjmccall wrote: > > > ebevhan wrote: > > > > rjmccall wrote: > > > > > Hmm. So adding a signed integer to an unsign

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

ebevhan marked an inline comment as done. ebevhan added inline comments. Comment at: lib/Sema/SemaChecking.cpp:7711-7715 + // It's an integer promotion if the destination type is the promoted + // source type. + return ICE->getCastKind() == CK_IntegralCast && + From->i

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

ebevhan updated this revision to Diff 176130. ebevhan added a comment. Rebased and addressed review comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51211/new/ https://reviews.llvm.org/D51211 Files: lib/Sema/SemaChecking.cpp test/Sema/format-strings-bitfield-promotion.c In

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

ebevhan added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:1304 +RHSTy = ResultTy; + } + rjmccall wrote: > Hmm. So adding a signed integer to an unsigned fixed-point type always > converts the integer to unsigned? That's a little weird, but on

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

ebevhan added a comment. In D53738#1310212 , @leonardchan wrote: > In D53738#1309171 , @ebevhan wrote: > > > In D53738#1308314 , @leonardchan > > wrote: > > > > > > Generall

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

ebevhan added a comment. In D53738#1308314 , @leonardchan wrote: > > Generally I think it's good! One final note; I assume we could technically > > reuse/rename the EmitFixedPointAdd function and use it to emit other binops > > when those are added? > >

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

ebevhan added a comment. In https://reviews.llvm.org/D53738#1305498, @leonardchan wrote: > @ebevhan Any more comments on this patch? It's strange, I feel like I've responded to the last comment twice now but there's nothing in Phab. Generally I think it's good! One final note; I assume we cou

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

ebevhan added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3385 + if (IsCommonSaturated) +CommonFixedSema.setSaturated(false); + ebevhan wrote: > Can EmitFixedPointConversion not determine this on its own? What I meant here was that rather

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

ebevhan added a comment. I'd be interested in seeing tests for two saturating unsigned _Fract with and without padding. If the padding case emits a uadd_sat, that seems wrong; uadd_sat doesn't saturate on the padding bit, but will saturate the whole number, which can result in invalid represen

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

ebevhan added a comment. > The only thing I didn't include in this patch were the suggested changes to > FixedPointSemantics which would indicate if the semantics were original from > an integer type. I'm not sure how necessary this is because AFAIK the only > time we would need to care if the

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

ebevhan added a comment. In https://reviews.llvm.org/D53738#1288333, @rjmccall wrote: > In https://reviews.llvm.org/D53738#1287123, @ebevhan wrote: > > > In https://reviews.llvm.org/D53738#1284213, @rjmccall wrote: > > > > > Not out of line with other features that significantly break with what's

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

ebevhan added a comment. In https://reviews.llvm.org/D53738#1284213, @rjmccall wrote: > >> 2. The question is easily answered by pointing at the language spec. The > >> language does not say that the operands are promoted to a common type; it > >> says the result is determined numerically from

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

ebevhan added a comment. In https://reviews.llvm.org/D53738#1283983, @rjmccall wrote: > In https://reviews.llvm.org/D53738#1283861, @ebevhan wrote: > > > In https://reviews.llvm.org/D53738#1283459, @rjmccall wrote: > > > > > Well, it could be passed around through most code as some sort of > > >

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

ebevhan added a comment. In https://reviews.llvm.org/D53738#1283459, @rjmccall wrote: > Well, it could be passed around through most code as some sort of > abstractly-represented intermediate "type" which could be either a QualType > or a fixed-point semantics. Sounds to me like you're descri

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

ebevhan added a comment. In https://reviews.llvm.org/D53738#1281332, @rjmccall wrote: > Well, maybe the cleanest solution would be to actually fold > `CompoundAssignOperator` back into `BinaryOperator` and just allow > `BinaryOperator` to optionally store information about the intermediate type

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

ebevhan added a comment. In https://reviews.llvm.org/D53738#1279069, @rjmccall wrote: > > The important difference would be that we separate the semantics of > > performing the conversion and the operation. I understand that adding a new > > type could be a bit more involved than baking the con

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

ebevhan added a comment. In https://reviews.llvm.org/D53738#1278078, @rjmccall wrote: > I don't think we should add *types* just for this, but if you need to make a > different kind of `BinaryOperator` that represents that the semantics aren't > quite the same (the same way that the compound as

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

ebevhan added a comment. I think I've already expressed that I'm not at all a fan of encoding the full-precision logic in the operations themselves and not explicitly in the AST. We already have (well, not yet, but we will) routines for emitting conversions to/from/between fixed-point types of

[PATCH] D53308: [Fixed Point Arithmetic] Fixed Point to Boolean Cast

ebevhan added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:2026 +return EmitScalarConversion(Visit(E), E->getType(), DestTy, +CE->getExprLoc()); rjmccall wrote: > Why are you pushing these casts through `Em

[PATCH] D53308: [Fixed Point Arithmetic] Fixed Point to Boolean Cast

ebevhan added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1032 + // them. + return Builder.CreateIsNotNull(Src); +} Is this comment true? I don't think EmitFixedPointConversion does this. Maybe I'm misinterpreting what it means.

[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

ebevhan added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:1019 + assert(!SrcType->isFixedPointType() && !DstType->isFixedPointType() && + "Use the TargetCodeGenInfo::emitFixedPoint family functions for " + "handling conversions involving fixed point

[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

ebevhan added a comment. I agree with John, after that I think it's fine for me. Repository: rC Clang https://reviews.llvm.org/D50616 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commi

[PATCH] D52286: [Intrinsic] Signed Saturation Intirnsic

ebevhan added inline comments. Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:1119 + case ISD::SSAT: +// Target legalization checked here? +Action = TargetLowering::Expand; leonardchan wrote: > ebevhan wrote: > > leonardchan wrote: > > > ebevhan wr

[PATCH] D52286: [Intrinsic] Signed Saturation Intirnsic

ebevhan added inline comments. Comment at: include/llvm/CodeGen/ISDOpcodes.h:264 +/// signed value is returned instead. +SSAT, + leonardchan wrote: > ebevhan wrote: > > leonardchan wrote: > > > ebevhan wrote: > > > > With the way the rest is written, it d

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

ebevhan added a comment. Another ping. Anyone up for reviewing this patch? Repository: rC Clang https://reviews.llvm.org/D51211 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

ebevhan added a comment. Ping. Repository: rC Clang https://reviews.llvm.org/D51211 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51426: [Sema] Prohibit function-scope compound literals with address spaces.

ebevhan added a comment. Thanks! I don't have commit access, so it would be appreciated if someone could commit it. https://reviews.llvm.org/D51426 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listi

[PATCH] D51426: [Sema] Prohibit function-scope compound literals with address spaces.

ebevhan updated this revision to Diff 163277. ebevhan edited the summary of this revision. ebevhan added a comment. Added Embedded-C clause quote. Fixed formatting. https://reviews.llvm.org/D51426 Files: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaExpr.cpp test/Sema/address_sp

[PATCH] D51426: [Sema] Prohibit function-scope compound literals with address spaces.

ebevhan added inline comments. Comment at: lib/Sema/SemaExpr.cpp:5745 +// C99 6.5.2.5p6: Function scope compound literals must have automatic +// storage which generally excludes address space-qualified ones. +Diag(LParenLoc, diag::err_compound_literal_with_address_sp

[PATCH] D51426: [Sema] Prohibit function-scope compound literals with address spaces.

ebevhan created this revision. ebevhan added reviewers: efriedma, rjmccall. Herald added a subscriber: cfe-commits. In C, compound literals in function scope are lvalues with automatic storage duration. This means that generally, they cannot be address space-qualified since you cannot have address

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

ebevhan created this revision. ebevhan added reviewers: krememek, rsmith. Herald added a subscriber: cfe-commits. The integer promotions apply to bitfields as well, but rather oddly. If you have a bitfield with a lower width than int, then the type of the member expression will be int regardless o

[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

ebevhan added a comment. In https://reviews.llvm.org/D50616#1204588, @leonardchan wrote: > Would it be simpler instead just to have the logic contained in the virtual > function for `TargetCodeGenInfo` as opposed to returning `nullptr` since any > custom target will end up overriding it anyway

[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

ebevhan added a comment. In https://reviews.llvm.org/D50616#1203446, @leonardchan wrote: > Sorry I forgot to address this also. Just to make sure I understand this > correctly since I haven't used these before: target hooks are essentially for > emitting target specific code for some operations

[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

ebevhan added a comment. I think I've mentioned this before, but I would like to discuss the possibility of adding a target hook(s) for some of these operations (conversions, arithmetic when it comes). Precisely matching the emitted saturation operation is virtually impossible for a target.

[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer

ebevhan added a comment. I think the solution to a lot of diagnostic and behavior issues with address spaces is to remove predication on OpenCL. It's a bit odd to have a language feature that is enabled out of the box regardless of langoptions (address spaces) but won't actually work properly u

[PATCH] D49945: [Fixed Point Arithmetic] Fix for FixedPointValueToString

ebevhan added inline comments. Comment at: lib/AST/Expr.cpp:788 FixedPointValueToString( - S, llvm::APSInt::getUnsigned(getValue().getZExtValue()), Scale, Radix); + S, llvm::APSInt::getUnsigned(getValue().getZExtValue()), Scale); return S.str(); U

[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer

ebevhan added a comment. When I try the test case on our downstream (and when compiling for our target with an extra flag that enables a bunch of OpenCL-related address space code), I get: ascrash.c:5:12: error: comparison between ('__attribute__((address_space(1))) char *' and '__attribute_

[PATCH] D49945: [Fixed Point Arithmetic] Fix for FixedPointValueToString

ebevhan accepted this revision. ebevhan added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang https://reviews.llvm.org/D49945 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi

[PATCH] D48661: [Fixed Point Arithmetic] Fixed Point Constant

ebevhan added a comment. Sorry for the late response, I've been away. LGTM, although my browser doesn't want to let me set Accepted on this. Comment at: unittests/Basic/FixedPointTest.cpp:380 +} + +// Check the value in a given fixed point sema overflows to the saturated max -

[PATCH] D48661: [Fixed Point Arithmetic] Fixed Point Constant

ebevhan added inline comments. Comment at: lib/Basic/FixedPoint.cpp:40 + if (DstWidth > Val.getBitWidth()) +Val = Val.extend(DstWidth); + if (Upscaling) leonardchan wrote: > ebevhan wrote: > > It should be possible to replace this with `extOrTrunc` and move

[PATCH] D48661: [Fixed Point Arithmetic] Fixed Point Constant

ebevhan added inline comments. Comment at: include/clang/Basic/FixedPoint.h:67 + // Convert this number to match the semantics provided. + void convert(const struct fixedPointSemantics &DstSema); + If this class is supposed to be used like APInt and APSInt, per

[PATCH] D48661: [Fixed Point Arithmetic] Fixed Point Constant

ebevhan added inline comments. Comment at: lib/Basic/FixedPoint.cpp:53 +// We can overflow here +unsigned ShiftAmt = DstScale - Scale; +if (Val < 0 && Val.countLeadingOnes() >= ShiftAmt) ebevhan wrote: > I think saturation can be modeled a bit better.

[PATCH] D48661: [Fixed Point Arithmetic] Fixed Point Constant

ebevhan added a comment. I also think it would be good with some unit tests for this class once the functionality and interface is nailed down. Comment at: include/clang/Basic/FixedPoint.h:31 + SatNoPadding, +}; + rjmccall wrote: > I figured you'd want this t

[PATCH] D48661: [Fixed Point Arithmetic] Fixed Point Constant

ebevhan added inline comments. Comment at: include/clang/Basic/FixedPoint.h:23 + +class FixedPointNumber { + public: rjmccall wrote: > The established naming convention here — as seen in `APInt`, `APFloat`, > `APValue`, etc. — would call this `APFixedPoint`. Ma

[PATCH] D48456: [Fixed Point Arithmetic] Casting between fixed point types and other arithmetic types

ebevhan added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:768 +if (CGF.getContext().getTargetInfo().unsignedFixedPointTypesHavePadding() && +Ty->isUnsignedFixedPointType()) { + unsigned NumBits = CGF.getContext().getTypeSize(Ty); r

[PATCH] D48456: [Fixed Point Arithmetic] Casting between fixed point types and other arithmetic types

ebevhan added inline comments. Comment at: include/clang/AST/ASTContext.h:1954 + llvm::APInt getFixedPointMin(QualType Ty) const; + llvm::APInt getFixedPointOne(QualType Ty) const; rjmccall wrote: > ebevhan wrote: > > rjmccall wrote: > > > Are these opaque bi

[PATCH] D48456: [Fixed Point Arithmetic] Casting between fixed point types and other arithmetic types

ebevhan added inline comments. Comment at: lib/AST/ExprConstant.cpp:9501 + return false; +return Success(Result.getInt() >> Scale, E); + } ebevhan wrote: > The shift here will not produce the correct rounding behavior for fixed-point > to integer conve

[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.

ebevhan added a comment. Herald added a subscriber: mikhail.ramalho. Ping. https://reviews.llvm.org/D46944 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48456: [Fixed Point Arithmetic] Casting between fixed point types and other arithmetic types

ebevhan added a comment. Would it be possible to add some form of target hook (perhaps to CodeGenABIInfo, which is already accessed with `getTargetHooks`) for fixed-point operations (maybe even some conversions)? As I've mentioned earlier, we emit both IR and intrinsics for many of these operat

[PATCH] D47630: [Sema] Allow creating types with multiple of the same addrspace.

ebevhan added a comment. Thanks! I do not have commit access, so it would be great if someone could commit this. https://reviews.llvm.org/D47630 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/

[PATCH] D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals

ebevhan accepted this revision. ebevhan added inline comments. This revision is now accepted and ready to land. Comment at: lib/Sema/SemaExpr.cpp:1248 + bool RHSFixed = RHSType->isFixedPointType(); + + if (LHSFixed && RHSFixed) { leonardchan wrote: > ebevhan wr

[PATCH] D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals

ebevhan added a comment. Just a couple more comments and then I think it looks good. We can discuss the conversion and comparison issues in later patches. Comment at: include/clang/AST/ASTContext.h:1951 + unsigned char getFixedPointScale(const QualType &Ty) const; + unsigned

[PATCH] D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals

ebevhan added inline comments. Comment at: lib/Sema/SemaExpr.cpp:1248 + bool RHSFixed = RHSType->isFixedPointType(); + + if (LHSFixed && RHSFixed) { leonardchan wrote: > leonardchan wrote: > > ebevhan wrote: > > > ebevhan wrote: > > > > leonardchan wrote: > > >

[PATCH] D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals

ebevhan added inline comments. Comment at: lib/Sema/SemaExpr.cpp:1248 + bool RHSFixed = RHSType->isFixedPointType(); + + if (LHSFixed && RHSFixed) { ebevhan wrote: > leonardchan wrote: > > ebevhan wrote: > > > leonardchan wrote: > > > > ebevhan wrote: > > > > >

[PATCH] D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals

ebevhan added inline comments. Comment at: include/clang/Basic/TargetInfo.h:89 + // corresponding unsaturated types. + unsigned char ShortAccumFBits, ShortAccumIBits; + unsigned char AccumFBits, AccumIBits; leonardchan wrote: > ebevhan wrote: > > leonardchan w

[PATCH] D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals

ebevhan added inline comments. Comment at: lib/AST/ExprConstant.cpp:9437 + } + return Success(-Value, E); +} This looks very suspect to me as well... This might not respect padding on types whose sign bit is not the MSB, such as _Fract. The same go

[PATCH] D46911: [Fixed Point Arithmetic] Addition of the remaining fixed point types and their saturated equivalents

ebevhan added inline comments. Comment at: test/Frontend/fixed_point_bit_widths.c:44 + +// CHECK-NEXT: @size_SsF = dso_local global i{{[0-9]+}} 2 +// CHECK-NEXT: @size_SF = dso_local global i{{[0-9]+}} 4 Wait; should these dso_local be `{{.*}}`? Repository:

[PATCH] D46911: [Fixed Point Arithmetic] Addition of the remaining fixed point types and their saturated equivalents

ebevhan accepted this revision. ebevhan added a comment. This revision is now accepted and ready to land. LGTM, but I'm not a code owner so maybe someone else should chime in if they have any input. Repository: rC Clang https://reviews.llvm.org/D46911 _

[PATCH] D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals

ebevhan added inline comments. Comment at: include/clang/Basic/TargetInfo.h:89 + // corresponding unsaturated types. + unsigned char ShortAccumFBits, ShortAccumIBits; + unsigned char AccumFBits, AccumIBits; leonardchan wrote: > ebevhan wrote: > > leonardchan w

[PATCH] D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals

ebevhan added inline comments. Comment at: lib/Lex/LiteralSupport.cpp:1187 + + uint64_t Base = (radix == 16) ? 2 : 10; + if (BaseShift > 0) { leonardchan wrote: > ebevhan wrote: > > I don't think loops are needed here. BaseShift is essentially Exponent so > >

[PATCH] D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals

ebevhan added inline comments. Comment at: include/clang/Basic/TargetInfo.h:89 + // corresponding unsaturated types. + unsigned char ShortAccumFBits, ShortAccumIBits; + unsigned char AccumFBits, AccumIBits; leonardchan wrote: > ebevhan wrote: > > I suspect it'

[PATCH] D47627: [ASTContext] Make getAddrSpaceQualType replace address spaces.

ebevhan added a comment. > Well, the documentation mismatch is worth fixing even if the code isn't. But > I think at best your use-case calls for weakening the assertion to be that > any existing address space isn't *different*, yeah. Alright, I'll give that a shot. > Separately, I'm not sure

[PATCH] D47630: [Sema] Allow creating types with multiple of the same addrspace.

ebevhan updated this revision to Diff 150683. ebevhan edited the summary of this revision. ebevhan added a comment. Added a warning for identical address spaces. https://reviews.llvm.org/D47630 Files: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaType.cpp test/Sema/address_space

[PATCH] D46911: [Fixed Point Arithmetic] Addition of the remaining fixed point types and their saturated equivalents

ebevhan added inline comments. Comment at: include/clang/Sema/DeclSpec.h:670 const PrintingPolicy &Policy); + bool SetTypeSpecSat(SourceLocation Loc, const char *&PrevSpec, + unsigned &DiagID); leonardchan wrote: > eb

[PATCH] D46911: [Fixed Point Arithmetic] Addition of the remaining fixed point types and their saturated equivalents

ebevhan added inline comments. Comment at: lib/Sema/SemaType.cpp:1612 + // Only fixed point types can be saturated + if (DS.isTypeSpecSat() && !IsFixedPointType) +S.Diag(DS.getTypeSpecSatLoc(), diag::err_invalid_saturation_spec) Also, this does not seem to

[PATCH] D46911: [Fixed Point Arithmetic] Addition of the remaining fixed point types and their saturated equivalents

ebevhan added a comment. Actually, wait! One last thing I missed. Comment at: include/clang/Sema/DeclSpec.h:670 const PrintingPolicy &Policy); + bool SetTypeSpecSat(SourceLocation Loc, const char *&PrevSpec, + unsigned &DiagID); ---

[PATCH] D47630: [Sema] Allow creating types with multiple of the same addrspace.

ebevhan added inline comments. Comment at: test/Sema/address_spaces.c:17 int *_AS1 _AS2 *Z; // expected-error {{multiple address spaces specified for type}} + int *_AS1 _AS1 *M; Anastasia wrote: > ebevhan wrote: > > bader wrote: > > > I think it might be

[PATCH] D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals

ebevhan added inline comments. Comment at: include/clang/AST/Type.h:6552 +// as a scaled integer. +std::string FixedPointValueToString(unsigned Radix, unsigned Scale, +uint64_t Val); This should probably take an APInt or APSInt

[PATCH] D46911: [Fixed Point Arithmetic] Addition of the remaining fixed point types and their saturated equivalents

ebevhan added a comment. Yes, it looks good to me. Repository: rC Clang https://reviews.llvm.org/D46911 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47627: [ASTContext] Make getAddrSpaceQualType replace address spaces.

ebevhan added a comment. There's actually a bit more to it than in these two patches. The complete diff to this function in our downstream Clang looks like this: QualType ASTContext::getAddrSpaceQualType(QualType T, unsigned AddressSpace) const { - QualType CanT = getCanonicalType(T);

[PATCH] D47627: [ASTContext] Make getAddrSpaceQualType replace address spaces.

ebevhan added a comment. I have this patch uploaded as well: https://reviews.llvm.org/D47630 I suspected that there might be cases for which we would try adding the same address space to a type, but I noticed that this method doesn't replace properly. I don't actually have an example of this, t

[PATCH] D47630: [Sema] Allow creating types with multiple of the same addrspace.

ebevhan added inline comments. Comment at: test/Sema/address_spaces.c:17 int *_AS1 _AS2 *Z; // expected-error {{multiple address spaces specified for type}} + int *_AS1 _AS1 *M; bader wrote: > I think it might be valuable to give a warning or remark to us

[PATCH] D47630: [Sema] Allow creating types with multiple of the same addrspace.

ebevhan created this revision. ebevhan added a reviewer: Anastasia. Herald added a subscriber: cfe-commits. The comment with the OpenCL clause about this clearly says: "No type shall be qualified by qualifiers for two or more different address spaces." This must mean that two or more qualifiers f

[PATCH] D47627: [ASTContext] Make getAddrSpaceQualType replace address spaces.

ebevhan created this revision. ebevhan added reviewers: rjmccall, arichardson. Herald added a subscriber: cfe-commits. The documentation for getAddrSpaceQualType says: "If T already has an address space specifier, it is silently replaced." The function did not do this; it asserted instead. Fix it

[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.

ebevhan updated this revision to Diff 149415. ebevhan edited the summary of this revision. ebevhan added a comment. Changed ArrayIndexTy back to LongLongTy and reverted the test change. https://reviews.llvm.org/D46944 Files: lib/StaticAnalyzer/Core/ProgramState.cpp lib/StaticAnalyzer/Core/R

[PATCH] D46911: [Fixed Point Arithmetic] Addition of the remaining fixed point types and their saturated equivalents

ebevhan added inline comments. Comment at: include/clang/AST/ASTContext.h:2882 + + QualType getCorrespondingSaturatedType(const QualType &Ty) const; }; This probably belongs up near the other predicates. Also I think it's more common to simply pass `QualType`

[PATCH] D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type

ebevhan added inline comments. Comment at: lib/Basic/TargetInfo.cpp:45 + AccumWidth = AccumAlign = 32; + LongAccumWidth = LongAccumAlign = 64; SuitableAlign = 64; rsmith wrote: > leonardchan wrote: > > leonardchan wrote: > > > ebevhan wrote: > > > > leonardc

[PATCH] D46911: [Fixed Point Arithmetic] Addition of the remaining fixed point types and their saturated equivalents

ebevhan added inline comments. Comment at: include/clang/AST/Type.h:6551 + +QualType getCorrespondingSaturatedType(const ASTContext &Context, + const QualType &Ty); These should probably be in ASTContext directly. =

[PATCH] D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type

ebevhan added inline comments. Comment at: lib/Basic/TargetInfo.cpp:45 + AccumWidth = AccumAlign = 32; + LongAccumWidth = LongAccumAlign = 64; SuitableAlign = 64; rsmith wrote: > jfb wrote: > > This seems weird because Targets which don't have these values f

[PATCH] D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type

ebevhan added a comment. Sorry for the late notice; I missed something. Comment at: include/clang/Basic/TokenKinds.def:393 +// ISO/IEC JTC1 SC22 WG14 N1169 Extension +KEYWORD(_Accum , KEYALL) + I believe that having KEYALL will enable the k

[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.

ebevhan added inline comments. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:89 SymMgr(context, BasicVals, alloc), MemMgr(context, alloc), -StateMgr(stateMgr), ArrayIndexTy(context.LongLongTy), +StateMgr(stateMgr), ArrayIndexTy

[PATCH] D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type

ebevhan added a comment. With the exception of the two inline comments, this looks good to me now! Comment at: include/clang/Basic/LangOptions.def:301 +LANGOPT(FixedPointEnabled, 1, 0, "Fixed point types are enabled") + Just a minor nit... The 'Enabled' is su

[PATCH] D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type

ebevhan added inline comments. Comment at: include/clang/Basic/TargetInfo.h:382 +// enough bits to fit the minumum. +if (getIntWidth() < MinSignedAccumDataBits) + return getLongWidth(); I'm not sure I agree with this interpretation. It's simply not c

<    1   2   3   4   >