[PATCH] D38479: Make -mgeneral-regs-only more like GCC's

[George Burgess IV via Phabricator via cfe-commits]

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