Removal of Global SRoA

Hi Fangrui,

In this patch you removed an optimization outright: rG129f466e222e

I didn’t see this get discussed, and the only justification seems to be that “it doesn’t does not trigger at all when bootstrapping clang”.

This is a spec optimization, and while that may not be important to Google, that is important to many people who use LLVM. Can you please revert this patch and discuss if further?

-Chris

Hi Chris,

[I took my second dose of vaccine yesterday and felt under the weather, so I may be slow on replies.]

Hi Fangrui,

In this patch you removed an optimization outright: rG129f466e222e

I didn’t see this get discussed, and the only justification seems to be that “it doesn’t does not trigger at all when bootstrapping clang”.

Sorry but I cannot agree with this. I opened
⚙ D101245 [GlobalOpt] Disable heap SROA when GEP of the only storer is used (Apr 24) for review and the removal was
my second attempt after I realized that it is non-trivial to address
potential miscompilation issues.

This is a spec optimization, and while that may not be important to Google, that is important to many people who use LLVM. Can you please revert this patch and discuss if further?

50027 – globalopt results in assertion (Instructions.h:942) was reported by Loris Reiff,
who is not associated with Google. I was helping fixing the issue as an
LLVM community member. The bug reported by Loris could cause an internal
compiler crash but I realized we could have miscompilation issues.

I added abort() in PerformHeapAllocSRoA() and tested an internal spec2006 - no file caused a clang crash.
So this optimization isn't triggered in spec2006.

Arthur Eubanks confirmed this optimization did not fire when bootstrapping clang.

I just checked llvm-test-suite as well - not triggered.

Given that this optimization was not triggered in any of
llvm-project/spec2006/llvm-test-suite and the optimization could cause
miscompilation issues, I'd prefer we don't revert the patch.
Once a regression is found, we can keep investigating.

Hello,

I am interested in this if this improves SPEC, which I didn’t know to be honest.

If this doesn’t trigger anymore, I am willing to look into that (not immediately, but at some point), but if the opinion is that the current state and implementation is beyond repair and need to be removed then that is interesting and probably best be discussed here as there are very few reviewers on those changes, so this message may not have reached all who are interested.

Cheers,
Sjoerd.