[PATCH] D38479: Make -mgeneral-regs-only more like GCC's
George Burgess IV via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu May 23 13:14:50 PDT 2019
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
More information about the cfe-commits
mailing list