[PATCH] D37254: [Sema] Disallow assigning record lvalues with nested const-qualified fields.

2017-09-08 Thread Bevin Hansson via Phabricator via cfe-commits
bevinh updated this revision to Diff 114340. bevinh marked an inline comment as done. bevinh added a comment. Added a diag note for NestedConstMember. https://reviews.llvm.org/D37254 Files: include/clang/AST/Expr.h include/clang/AST/Type.h include/clang/Basic/DiagnosticSemaKinds.td

[PATCH] D37254: [Sema] Disallow assigning record lvalues with nested const-qualified fields.

2017-08-29 Thread Bevin Hansson via Phabricator via cfe-commits
bevinh created this revision. According to C99 6.3.2.1p1, structs and unions with nested const-qualified fields (that is, const-qualified fields declared at some recursive level of the aggregate) are not modifiable lvalues. However, Clang permits assignments of these lvalues. With this patch, we

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

2018-05-16 Thread Bevin Hansson via Phabricator via cfe-commits
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 /

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

2018-05-16 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-05-16 Thread Bevin Hansson via Phabricator via cfe-commits
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 /

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

2018-05-23 Thread Bevin Hansson via Phabricator via cfe-commits
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] D46927: [Fixed Point Arithmetic] Augmented Assignment for Fixed Point Types

2018-05-23 Thread Bevin Hansson via Phabricator via cfe-commits
ebevhan added inline comments. Comment at: include/clang/AST/Type.h:6577 + +inline unsigned getFixedPointFBits(const QualType ) { + return getFixedPointFBits(*Ty); As I mentioned in another patch, this should be in ASTContext and ask TargetInfo for the

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

2018-05-23 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-05-23 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-05-23 Thread Bevin Hansson via Phabricator via cfe-commits
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

2018-05-23 Thread Bevin Hansson via Phabricator via cfe-commits
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] D46917: [Fixed Point Arithmetic] Comparison and Unary Operations for Fixed Point Types

2018-05-23 Thread Bevin Hansson via Phabricator via cfe-commits
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:

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

2018-05-23 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-05-23 Thread Bevin Hansson via Phabricator via cfe-commits
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] D46944: [analyzer] Use sufficiently large types for index/size calculation.

2018-05-24 Thread Bevin Hansson via Phabricator via cfe-commits
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),

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

2018-05-25 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-05-25 Thread Bevin Hansson via Phabricator via cfe-commits
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),

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

2018-05-24 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-05-21 Thread Bevin Hansson via Phabricator via cfe-commits
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:

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

2018-06-11 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-06-11 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-06-12 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-06-07 Thread Bevin Hansson via Phabricator via cfe-commits
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

2018-06-07 Thread Bevin Hansson via Phabricator via cfe-commits
ebevhan added inline comments. Comment at: include/clang/Sema/DeclSpec.h:670 const PrintingPolicy ); + bool SetTypeSpecSat(SourceLocation Loc, const char *, + unsigned ); leonardchan wrote: > ebevhan wrote: > > This

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

2018-06-14 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-06-18 Thread Bevin Hansson via Phabricator via cfe-commits
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

2018-06-13 Thread Bevin Hansson via Phabricator via cfe-commits
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

2018-06-13 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-06-13 Thread Bevin Hansson via Phabricator via cfe-commits
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

2018-06-13 Thread Bevin Hansson via Phabricator via cfe-commits
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

2018-06-13 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-06-14 Thread Bevin Hansson via Phabricator via cfe-commits
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

2018-06-19 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-05-31 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-05-31 Thread Bevin Hansson via Phabricator via cfe-commits
ebevhan added inline comments. Comment at: include/clang/AST/Type.h:6551 + +QualType getCorrespondingSaturatedType(const ASTContext , + const QualType ); These should probably be in ASTContext directly.

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

2018-06-04 Thread Bevin Hansson via Phabricator via cfe-commits
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,

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

2018-06-01 Thread Bevin Hansson via Phabricator via cfe-commits
ebevhan added inline comments. Comment at: include/clang/AST/ASTContext.h:2882 + + QualType getCorrespondingSaturatedType(const QualType ) 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.

2018-06-01 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-06-01 Thread Bevin Hansson via Phabricator via cfe-commits
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: > > > >

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

2018-06-01 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-06-01 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-06-01 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-06-05 Thread Bevin Hansson via Phabricator via cfe-commits
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

2018-06-07 Thread Bevin Hansson via Phabricator via cfe-commits
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

2018-06-07 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-06-07 Thread Bevin Hansson via Phabricator via cfe-commits
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

2018-06-07 Thread Bevin Hansson via Phabricator via cfe-commits
ebevhan added a comment. Actually, wait! One last thing I missed. Comment at: include/clang/Sema/DeclSpec.h:670 const PrintingPolicy ); + bool SetTypeSpecSat(SourceLocation Loc, const char *, + unsigned ); This

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

2018-05-30 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-06-25 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-06-26 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-06-26 Thread Bevin Hansson via Phabricator via cfe-commits
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] D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals

2018-06-19 Thread Bevin Hansson via Phabricator via cfe-commits
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 ) const; + unsigned

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

2018-06-20 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-06-27 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-06-27 Thread Bevin Hansson via Phabricator via cfe-commits
ebevhan added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:768 +if (CGF.getContext().getTargetInfo().unsignedFixedPointTypesHavePadding() && +Ty->isUnsignedFixedPointType()) { + unsigned NumBits = CGF.getContext().getTypeSize(Ty);

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

2018-07-03 Thread Bevin Hansson via Phabricator via cfe-commits
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 ); + If this class is supposed to be used like APInt and APSInt, perhaps

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

2018-08-06 Thread Bevin Hansson via Phabricator via cfe-commits
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] D50278: [Sema] Fix for crash on conditional operation with address_space pointer

2018-08-06 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-08-06 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-08-07 Thread Bevin Hansson via Phabricator via cfe-commits
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

[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

2018-08-16 Thread Bevin Hansson via Phabricator via cfe-commits
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

2018-08-17 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-08-06 Thread Bevin Hansson via Phabricator via cfe-commits
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();

[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

2018-08-20 Thread Bevin Hansson via Phabricator via cfe-commits
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] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

2018-08-24 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-08-29 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-09-07 Thread Bevin Hansson via Phabricator via cfe-commits
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.

2018-08-30 Thread Bevin Hansson via Phabricator via cfe-commits
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,

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

2018-08-30 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-07-04 Thread Bevin Hansson via Phabricator via cfe-commits
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

[PATCH] D45865: [Sema] Emit -Warray-bounds for multiple levels of subscript expressions.

2018-04-20 Thread Bevin Hansson via Phabricator via cfe-commits
ebevhan created this revision. ebevhan added reviewers: danielmarjamaki, aaron.ballman. Herald added a subscriber: cfe-commits. This patch has CheckArrayBounds recurse into ArraySubscriptExprs and MemberExprs, giving warnings for invalid indices for every level of subscript instead of just the

[PATCH] D45865: [Sema] Emit -Warray-bounds for multiple levels of subscript expressions.

2018-04-24 Thread Bevin Hansson via Phabricator via cfe-commits
ebevhan updated this revision to Diff 143684. ebevhan added a comment. Removed local variable and added some more to the test. https://reviews.llvm.org/D45865 Files: lib/Sema/SemaChecking.cpp test/SemaCXX/array-bounds.cpp test/SemaCXX/constant-expression-cxx11.cpp Index:

[PATCH] D45865: [Sema] Emit -Warray-bounds for multiple levels of subscript expressions.

2018-04-24 Thread Bevin Hansson via Phabricator via cfe-commits
ebevhan added a comment. Thank you! I do not have commit rights, so if someone could commit this that would be great. https://reviews.llvm.org/D45865 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

2018-06-28 Thread Bevin Hansson via Phabricator via cfe-commits
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`.

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

2018-06-29 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-06-29 Thread Bevin Hansson via Phabricator via cfe-commits
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

[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

2018-10-15 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-10-16 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-10-22 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-10-29 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-10-30 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-11-01 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-11-01 Thread Bevin Hansson via Phabricator via cfe-commits
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

2018-10-31 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-11-05 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-10-26 Thread Bevin Hansson via Phabricator via cfe-commits
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

2018-11-06 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-11-15 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-11-14 Thread Bevin Hansson via Phabricator via cfe-commits
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] D50616: [Fixed Point Arithmetic] FixedPointCast

2018-10-10 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-08-30 Thread Bevin Hansson via Phabricator via cfe-commits
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

[PATCH] D52286: [Intrinsic] Signed Saturation Intirnsic

2018-09-26 Thread Bevin Hansson via Phabricator via cfe-commits
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

[PATCH] D52286: [Intrinsic] Signed Saturation Intirnsic

2018-09-25 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2018-09-20 Thread Bevin Hansson via Phabricator via cfe-commits
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] D53738: [Fixed Point Arithmetic] Fixed Point Addition

2018-11-16 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2019-01-21 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2019-01-21 Thread Bevin Hansson via Phabricator via cfe-commits
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

2019-01-22 Thread Bevin Hansson via Phabricator via cfe-commits
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

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

2019-01-15 Thread Bevin Hansson via Phabricator via cfe-commits
ebevhan added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:7568 +static bool EvaluateFixedPoint(const Expr *E, APValue , EvalInfo ) { + if (E->getType()->isFixedPointType()) { This could provide an APFixedPoint rather than APValue.

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

2019-01-18 Thread Bevin Hansson via Phabricator via cfe-commits
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

  1   2   3   4   >