[PATCH] D124071: [NewGVN][LoadCoercion][1/3] Add support for load coercion between store and load instructions

Tue Sep 20 10:41:18 PDT 2022

asbirlea added a subscriber: momchil.velikov.
asbirlea added a comment.

Quick note, since updates on this until now were offline.
After the fix in D130910 <https://reviews.llvm.org/D130910> that Konstantina did, I was able to do some testing for NewGVN and haven't run into any crashes, which was a first. This doesn't mean that the current NewGVN is fully correct, just that in the testing I did nothing was obviously broken. This does mean I can test the correctness of this stack of patches on the same workloads.
I tested the previous iteration, followed up with reproducers for issues found and I'm testing the current diff.

Regarding:

In D124071#3585047 <https://reviews.llvm.org/D124071#3585047>, @RKSimon wrote:

> @fhahn Do you we have anyone leading the GVN/NewGVN work these days do you know? I can't see anything in CODE_OWNERS

I'm currently focusing more on testing the GVN&MSSA patches by @momchil.velikov, than getting NewGVN up to parity.
After the above fix, I did some performance testing with NewGVN turned on vs GVN and it's pretty terrible. To give a rough idea, I'm seeing 20-70% regression for running time, and 30% compile-time regression (due to the compiler being bootstrapped; newgvn compiling the compiler makes the new compiler 30% slower). The biggest factor are the PRE optimizations that GVN does, but even comparing a single stage build NewGVN to GVN with PRE disabled, there are still a few test-suites that show up to 60% regressions.
On the flip side, comparing NewGVN vs GVN with no PRE, there are a couple of big improvements on ARM, so it's quite possible there are gains to harness for specific use cases and/or architectures, but there's a lot of work left to make it a replacement for GVN in general.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124071