[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] 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] 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. 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] D46944: [analyzer] Use sufficiently large types for index/size calculation.

ebevhan created this revision. ebevhan added reviewers: dergachev.a, dcoughlin. Herald added subscribers: cfe-commits, a.sidorin, szepet, xazax.hun. Herald added a reviewer: george.karpenkov. RegionStoreManager::getSizeInElements used 'int' for size calculations, and ProgramState::assumeInBound fe

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

ebevhan added inline comments. Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1344 // This is a signed value, since it's used in arithmetic with signed indices. - return svalBuilder.makeIntVal(RegionSize / EleSize, false); + return svalBuilder.makeIntVal(RegionSize / El

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

ebevhan added inline comments. Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1344 // This is a signed value, since it's used in arithmetic with signed indices. - return svalBuilder.makeIntVal(RegionSize / EleSize, false); + return svalBuilder.makeIntVal(RegionSize / El

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

ebevhan updated this revision to Diff 147738. ebevhan edited the summary of this revision. ebevhan added a comment. Made ArrayIndexTy into ssize_t, consolidated the tests and fixed the test that was failing. https://reviews.llvm.org/D46944 Files: include/clang/StaticAnalyzer/Core/PathSensiti

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

ebevhan added inline comments. Comment at: lib/AST/ASTContext.cpp:1775 +case BuiltinType::UShortAccum: Width = Target->getShortWidth(); Align = Target->getShortAlign(); Please give the types their own width and alignment accessors/variables in

[PATCH] D46960: [Fixed Point Arithmetic] Predefined Precision Macros

ebevhan added inline comments. Comment at: include/clang/Lex/Preprocessor.h:185 + // Fractional bits of _Accum types + IdentifierInfo *Ident__SACCUM_FBIT__;// __SACCUM_FBIT__ + IdentifierInfo *Ident__ACCUM_FBIT__; // __ACCUM_FBIT__ You

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

ebevhan added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:206 +def err_invalid_saturation_spec : Error<"'%0' cannot be saturated. Only _Fract and _Accum can.">; def err_invalid_sign_spec : Error<"'%0' cannot be signed or unsigned">; ---

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

ebevhan added a comment. You should not define the fixed-point precision as compiler macros at build configure time. The number of fractional bits (the scaling factor) should be added to TargetInfo as target-configurable variables/accessors, and an accessor should be added to ASTContext (we cal

[PATCH] D46917: [Fixed Point Arithmetic] Comparison and Unary Operations for Fixed Point Types

ebevhan added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:2141 + case BuiltinType::ShortAccum: +fbits = BUILTIN_SACCUM_FBIT; +break; Please see my comments on other patches about these values. Comment at: lib/

[PATCH] D46917: [Fixed Point Arithmetic] Comparison and Unary Operations for Fixed Point Types

ebevhan added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:2178 +llvm::Value *amt = +llvm::ConstantInt::get(value->getType(), 1 << fbits, + /*isSigned=*/type->isSignedFixedPointType()); ebevhan wrote: > Use a

[PATCH] D46927: [Fixed Point Arithmetic] Augmented Assignment for Fixed Point Types

ebevhan added inline comments. Comment at: include/clang/AST/Type.h:6577 + +inline unsigned getFixedPointFBits(const QualType &Ty) { + return getFixedPointFBits(*Ty); As I mentioned in another patch, this should be in ASTContext and ask TargetInfo for the infor

[PATCH] D46986: [Fixed Point Arithmetic] Validation Test for Fixed Point Binary Operations and Saturated Addition

ebevhan added a comment. I cannot say that I'm pleased with the CodeGen emission of the operations as pure IR. I can only assume that you do not have hardware specifically tailored for these operations, as matching this type of code ought to be quite difficult after optimization is performed.

[PATCH] D46986: [Fixed Point Arithmetic] Validation Test for Fixed Point Binary Operations and Saturated Addition

ebevhan added a comment. Also, this patch and all of the following 'Validation Test' patches do much more than just add tests, they add plenty of functionality as well. In general, it's quite difficult to tell which patches add what. I think it would be much clearer if the patches that claim to

[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 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

[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] 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] 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] 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] 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 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 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] 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] 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] 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] 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 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] 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] 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] 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: > 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/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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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 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] 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] 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] 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] 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: > > leonardchan w

[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] 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] 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] 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/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: 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 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 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] 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] 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] 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] 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] 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] 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] 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. 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#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] D55868: [Fixed Point Arithmetic] Fixed Point Addition Constant Expression Evaluation

ebevhan added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:7568 +static bool EvaluateFixedPoint(const Expr *E, APValue &Result, EvalInfo &Info) { + if (E->getType()->isFixedPointType()) { This could provide an APFixedPoint rather than APValue.

[PATCH] D56900: [Fixed Point Arithmetic] Fixed Point and Integer Conversions

ebevhan added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:9825 +if (Result.isSigned() && !DstSigned) { + Overflow = Result < 0 || Result.ugt(DstMax); +} else if (Result.isUnsigned() && DstSigned) { The `Result < 0` is more clearly exp

[PATCH] D56900: [Fixed Point Arithmetic] Fixed Point and Integer Conversions

ebevhan added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:9825 +if (Result.isSigned() && !DstSigned) { + Overflow = Result < 0 || Result.ugt(DstMax); +} else if (Result.isUnsigned() && DstSigned) { leonardchan wrote: > ebevhan wrote:

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

ebevhan added a comment. I'm a bit late to the party here, it was an older patch so I wasn't watching this one. I get a bit miffed when address space related features get locked behind langoptions that aren't strictly address spaces. I know that there are currently a couple code snippets which

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

ebevhan added a comment. Okay, sounds good! I'm not a C++ expert, but I'd be happy to look at the patches when they're up. Haven't done much serious testing on my end so far, but from what I've seen the patches so far look good! Repository: rC Clang CHANGES SINCE LAST ACTION https://revie

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

ebevhan added a comment. I think the code that comes to mind is mostly like in `GetTypeSourceInfoForDeclarator`: LangAS AS = (ASIdx == LangAS::Default ? LangAS::opencl_generic : ASIdx); It's behind an OpenCLCPlusPlus guard so it won't add generic on anything if it's not OpenCL++, but t

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

ebevhan added inline comments. Comment at: lib/Sema/SemaOverload.cpp:5151 + if (FromTypeCanon.getQualifiers().hasAddressSpace()) { +Qualifiers QualsImplicitParamType = ImplicitParamType.getQualifiers(); I tested this patch with some kludges to let it parse

[PATCH] D56900: [Fixed Point Arithmetic] Fixed Point and Integer Conversions

ebevhan added inline comments. Comment at: clang/test/Frontend/fixed_point_conversions.c:437 + // DEFAULT-NEXT: [[RES:%[a-z0-9]+]] = trunc i39 [[SATMIN]] to i16 + // DEFAULT-NEXT: store i16 [[RES]], i16* %sat_sa, align 2 + leonardchan wrote: > ebevhan wrote: >

[PATCH] D57219: [Fixed Point Arithmetic] Fixed Point Comparisons

ebevhan added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:141 + (RHSType->isFixedPointType() && + LHSType->isFixedPointOrIntegerType()); +} Can't it just be `LHSType->isFixedPointType() || RHSType->isFixedPointType

[PATCH] D57226: [Fixed Point] [AST] Add an AST serialization code for fixed-point literals.

ebevhan created this revision. ebevhan added reviewers: leonardchan, rjmccall. Herald added a subscriber: cfe-commits. ebevhan marked an inline comment as done. ebevhan added inline comments. Comment at: include/clang/Serialization/ASTBitCodes.h:1637 + /// A FixedPointLitera

[PATCH] D57226: [Fixed Point] [AST] Add an AST serialization code for fixed-point literals.

ebevhan marked an inline comment as done. ebevhan added inline comments. Comment at: include/clang/Serialization/ASTBitCodes.h:1637 + /// A FixedPointLiteral record. + EXPR_FIXEDPOINT_LITERAL, + I'm unsure if this is the right location for a new code. T

[PATCH] D57219: [Fixed Point Arithmetic] Fixed Point Comparisons

ebevhan added inline comments. Comment at: clang/test/Frontend/fixed_point_comparisons.c:56 + +void TestComparisons() { + short _Accum sa; leonardchan wrote: > ebevhan wrote: > > Missing saturating and saturating/non-saturating comparisons. I'd like to > > see

[PATCH] D57464: Generalize method overloading on addr spaces to C++

ebevhan added inline comments. Comment at: lib/Parse/ParseDeclCXX.cpp:2309 + attr.getScopeName(), attr.getScopeLoc(), &AU, + attr.getNumArgs(), ParsedAttr::AS_GNU); +attr.setInvalid(); Are

[PATCH] D57464: Generalize method overloading on addr spaces to C++

ebevhan added inline comments. Comment at: lib/Parse/ParseDeclCXX.cpp:2313 +} + } Anastasia wrote: > Anastasia wrote: > > ebevhan wrote: > > > Is there a reason that the attributes are parsed here and not in > > > `ParseFunctionDeclarator` like the rest o

[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#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#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#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. > 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. 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 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. 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 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 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 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] 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] 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] 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 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] 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

  1   2   3   4   >