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
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
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
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 /
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.
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
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
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
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
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">;
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:
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
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
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),
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
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),
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
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:
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
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
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
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
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
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
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:
> >
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
> >
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
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:
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
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
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:
> > > >
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
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
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.
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,
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`
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
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:
> > > >
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
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
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
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);
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
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
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
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
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
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
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
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
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
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
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
ebevhan added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:768
+if (CGF.getContext().getTargetInfo().unsignedFixedPointTypesHavePadding()
&&
+Ty->isUnsignedFixedPointType()) {
+ unsigned NumBits = CGF.getContext().getTypeSize(Ty);
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
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
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
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
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
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.
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
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();
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
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
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
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
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,
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
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
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
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:
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
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`.
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
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
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
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
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
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
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
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
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
> >
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
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
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
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
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
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
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
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
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
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
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
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
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
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:
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
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.
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
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
1 - 100 of 318 matches
Mail list logo
