george.burgess.iv added a comment.
Thanks for the feedback!
> With this patch, do we pass the general-regs-only attribute to the backend?
> If so, would that be the attribute we'd want to check to emit errors from the
> backend from any "accidental" floating-point operations?
Yeah, the current design is for us to pass +general-regs-only as a target
'feature' per function. Given that there's no code to actually handle that at
the moment, I've put a FIXME in its place. Please let me know if there's a
better way to go about this.
================
Comment at: clang/include/clang/Basic/LangOptions.def:143
LANGOPT(RelaxedTemplateTemplateArgs, 1, 0, "C++17 relaxed matching of template
template arguments")
+LANGOPT(GeneralOpsOnly , 1, 0, "Whether to diagnose the use of
floating-point or vector operations")
----------------
void wrote:
> Everywhere else you use "general regs only" instead of "ops". Should that be
> done here?
Yeah, I'm not sure why I named it `Ops`. Fixed
================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:7840
+
+ const auto *StructTy = Ty.getCanonicalType()->getAsStructureType();
+ if (!StructTy)
----------------
efriedma wrote:
> Do you really want to enforce isStruct() here? That's types declared with
> the keyword "struct".
Good catch -- generalized this.
================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:7857
+
+ return llvm::any_of(StructTy->getDecl()->fields(), [](const FieldDecl *FD) {
+ return typeHasFloatingOrVectorComponent(FD->getType());
----------------
efriedma wrote:
> Do we have to be concerned about base classes here?
Yup. Added tests for this, too
================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:7951
+ // they don't have to write out memcpy() for simple cases. For that reason,
+ // it's very limited in what it will detect.
+ //
----------------
efriedma wrote:
> We don't always lower struct copies to memcpy(); I'm not sure this is safe.
I see; removed. If this check ends up being important (it doesn't seem to be in
local builds), we can revisit. :)
================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:8003
+ !isRValueOfIllegalType(E) &&
+ E->isConstantInitializer(S.getASTContext(), /*ForRef=*/false)) {
+ resetDiagnosticState(InitialState);
----------------
efriedma wrote:
> Just because we can constant-fold an expression, doesn't mean we will,
> especially at -O0.
Are there any guarantees that we offer along these lines? The code in
particular that this cares about boils down to a bunch of integer literals
doing mixed math with FP literals, all of which gets casted to an `int`.
Conceptually, it seems silly to me to emit an addition for something as
straightforward as `int i = 1 + 2.0;`, even at -O0, though I totally agree that
you're right, and codegen like this is reasonable at -O0:
https://godbolt.org/z/NS0L17
(This also brings up a good point: this visitor probably shouldn't be run on
`IsConstexpr` expressions; fixed that later on)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D38479/new/
https://reviews.llvm.org/D38479
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
