The backstory of this topic is not related to GVN at all, so let me start from the beginning. This rather silly example is vectorized by GCC-AArch64, but not by LLVM, and this was bothering us:
void BeamFormWeights(int a[20000], int beam) {
for (int i = 0; i < 10000; ++i) {
if (i == beam)
a[2 * i] = 0;
else
a[2 * i] = 1;
}
}
While investigating why this didn’t vectorize, we found that GVN was being too eager in turning 2*i to 2*beam within the conditional. After GVN, the SCEVs of both array operations diverge wildly, and LoopAccessAnalysis fails to analyze this. I tried patching LAA initially, but my patch turned out to be a mess. Then, we analyzed what GCC does with this example, and it actually sinks the store outside the conditional, and we thought that this is perhaps the right solution to the problem. We investigated a small lesser-known pass called MergedLoadStoreMotion, which is supposed to do this sinking, but found that it was far too simplistic. Then, we stumbled across GVNSink which does what it promises, and the example is successfully vectorized with -enable-gvn-sink.
Now, I have been investigating GVNSink for a few hours, and it seems to be a profitable pass in all cases. I didn’t find any discussion online about why it wasn’t turned on by default, so I thought I’d ask here: the LLVM unit tests have a bunch of mysterious failures related to coroutines when it’s turned on, and there are a handful of very old bugs reported against it on GitHub. There have been very few non-NFC commits to GVNSink ever since it was introduced six years ago. Given these problems, a list of concrete action items would probably be too much to hope for, but some comments from people who have played with it would be helpful.
It’s probably worth a bug scrub on those issues you’ve identified. Have you tried to reproduce the issue? If they still exist and a reproducible, perhaps they MUST be fixed first. Otherwise if they’re not reproducible, perhaps it’s time to close those reports as such. Then it would perhaps make sense to enable at that point.
Have you done the git archeology to see who added -enable-gvn-sink? Have you contacted them?
Perhaps posting a link to this discussion on ⚙ D24805 [GVNSink] Initial GVNSink prototype and asking for feedback might get the right people to point you in the right direction?
Hi, I haven’t really played with it but I recently noticed that GVNSink was beneficial for code size at Oz optim level on both AOSP full build and SPEC benchmarks. Same for GVNHoist (-0.1/-0.2%)
@nikic’s patch was reverted, since it was causing regressions on SPEC (albeit, for a rather silly reason). I think we have one clear motivating example for GVNSink, but not much more. I did a quick benchmark run, and turning it on results in some serious regressions:
We’ve been using GVNSink and GVNHoist with -Oz because it helps with binary size and haven’t encountered any issues so far. I’d be supportive of enabling those passes by default in the -Oz pipeline where the potential performance regression isn’t really a concern.