Hi Chris,
[I took my second dose of vaccine yesterday and felt under the weather, so I may
be slow on replies.]
On 2021-05-18, Chris Lattner wrote:>Hi Fangrui,
>
>In this patch you removed an optimization outright:
https://reviews.llvm.org/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
https://reviews.llvm.org/D101245 (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?
https://bugs.llvm.org/show_bug.cgi?id=50027 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.