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

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

> Does this have any significant impact on -fsyntax-only performance?

I'm sure there are pathological cases where this hurts perf, but my intuition tells me that we won't get bitten badly by any of them in the real world. It should be a branch per cast + full expr for people who don't use it. For those who do, we're walking 2 types for each cast (plus maybe constexpr evaluations for casts from FP/vec to non-FP/vec values), and walking the types for each FullExpr.

Again, you can likely craft code that makes this expensive (which we can likely fix with strategically-placed caches), but being bitten by that in practice seems somewhat unlikely to me. Happy to try and add caching and/or take numbers with caches if you'd like.

To evaluate the build time impact of this, I did 20 builds of aarch64 Linux with/without this patch. Of the metrics I tracked (memory, system/user/wall time), all reported differences were < their stdev except for user time. User time regression with this patch was 0.37%, with a stdev of 0.12%. Happy to try to gather -fsyntax-only numbers if there's a simple way to do that.



================
Comment at: clang/lib/Driver/ToolChains/Arch/AArch64.cpp:194
     Features.push_back("-crypto");
     Features.push_back("-neon");
+    // FIXME: Ideally, we'd also like to pass a `+general-regs-only` feature,
----------------
efriedma wrote:
> Do we need to get rid of the "-neon" etc.?
Yeah, good call. Looks like I forgot a "RUN:" in my tests checking for whether or not asm works, so the thing that should've caught that wasn't enabled. :)


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:7997
+  bool isRValueOfIllegalType(const Expr *E) const {
+    return E->getValueKind() != VK_LValue && !isGeneralType(E);
+  }
----------------
efriedma wrote:
> Should this be `E->isRValue()`?  Not that the difference really matters for C, but it affects rvalue references in C++.
Yeah, flipped to that and added tests -- thanks


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

https://reviews.llvm.org/D38479