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

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Apologies for the latency of my updates.

> Something I ran into when reviewing https://reviews.llvm.org/D62639 is that on AArch64, for varargs functions, we emit floating-point stores when noimplicitfloat is specified. That seems fine for -mno-implicit-float, but maybe not for -mgeneral-regs-only?

Interesting. Yeah, -mgeneral-regs-only definitely doesn't want to use FP for varargs calls. As discussed offline, this doesn't appear to be an issue today, and is something we can look into in the future.



================
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.
+  //
----------------
george.burgess.iv wrote:
> 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. :)
(readded as noted)


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:8003
+        !isRValueOfIllegalType(E) &&
+        E->isConstantInitializer(S.getASTContext(), /*ForRef=*/false)) {
+      resetDiagnosticState(InitialState);
----------------
efriedma wrote:
> george.burgess.iv wrote:
> > 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)
> On trunk, we now have the notion of a ConstantExpr; this represents an expression which the language guarantees must be constant-evaluated.  For example, initializers for static variables in C are always constant-evaluated.
> 
> (On a related note, now that we have ConstantExpr, the IsConstexpr operand to ActOnFinishFullExpr probably isn't necessary.)
> 
> Beyond that, no, we don't really have any guarantees.  We may or may not try to constant-evaluate an expression, depending on whether we think it'll save compile-time.  For example, we try to fold branch conditions to avoid emitting the guarded block of code, but we don't try to fold the initialization of an arbitrary variable.
> 
> I don't think we want to introduce any additional guarantees here, if we can avoid it.
Resolving per offline discussion; anything wrapped in `ConstantExpr` is no longer checked, and we no longer try to constant evaluate anything else.

> On a related note, now that we have ConstantExpr, the IsConstexpr operand to ActOnFinishFullExpr probably isn't necessary

Looks like the lack of `IsConstexpr` fails to save us from one case:

```
constexpr float x = 1.;
```

In this context, all we have to work with from this point is an `IsConstexpr` `IntegerLiteral`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D38479/new/

https://reviews.llvm.org/D38479