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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to