[PATCH] D156458: GVNSink: add test to show GVN-aware sinking

[Ramkumar Ramachandra via Phabricator via llvm-commits]

artagnon added inline comments.


================
Comment at: llvm/test/Transforms/GVNSink/gvn-awareness.ll:50
+  %beam.addr = alloca i32, align 4
+  %i = alloca i32, align 4
+  store ptr %a, ptr %a.addr, align 8
----------------
nikic wrote:
> artagnon wrote:
> > xbolva00 wrote:
> > > artagnon wrote:
> > > > nikic wrote:
> > > > > Please at least run this through SROA. The test is a weird mix of completely unoptimized IR plus a GVN replacement.
> > > > When I run it through SROA, `gvn-sink` no longer works.
> > > Maybe also add phaseordering test?
> > > 
> > > If this fails to work after sroa, than this optimization will not work anyway with -O2/-O3
> > Sorry, what do you mean by a phaseordering test?
> A phase ordering test checks the whole `O3` pipeline. In cases like these where optimization depends on precise pass ordering, a phase ordering test ensures that the correct pass interaction is preserved going forward.
> 
> 
> I've committed a phase ordering test for your motivating case in https://github.com/llvm/llvm-project/commit/bbe2887f5e9ca005b0f1b96c858969ee3ba646f4 and submitted D156532 for a possible fix that does not rely on GVNSink.
Thanks a lot for this! Since the motivating example is fixed, and since `gvn-sink` only seems to cause regressions on benchmarks, I'm tempted to stop working on `gvn-sink`. Do you think we should abandon this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156458