This is an archive of the discontinued LLVM Phabricator instance.

[SROA] Speculate around PHIs: allow load to be in a unique-successor unique-predecessor block
AbandonedPublic

Authored by lebedev.ri on Mar 22 2021, 3:00 PM.

Details

Summary

There's 2009-02-20-InstCombine-SROA.ll InstCombine test that uses SROA,
and it breaks if i change InstCombine to split up inttoptr into bitcast(inttoptr).
The cause is, if we have a PHI and a load of that PHI, InstCombine may sink the load
into unique successor block. And isSafePHIToSpeculate() bails out on that.

So extend isSafePHIToSpeculate() to support traversing through such blocks.

Diff Detail

Event Timeline

lebedev.ri created this revision.Mar 22 2021, 3:00 PM
lebedev.ri requested review of this revision.Mar 22 2021, 3:00 PM
nikic added a subscriber: nikic.Mar 22 2021, 3:14 PM

If there is only one successor, and the one successor has only one predecessor, wouldn't these blocks be merged by SimplifyCFG? I'm not sure 2009-02-20-InstCombine-SROA.ll is really testing something meaningful because the specific -instcombine -sroa pass combination shouldn't occur in the wild. The primary SROA passes run before InstCombine, and the secondary post-unroll pass has so many other passes scheduled in between that reasoning about this in terms of just -instcombine -sroa is not very helpful.

I'm not really opposed to this change, but I'm also not sure it's necessary. Maybe in this case simply deleting the test case is the right thing to do?

I'm not really opposed to this change, but I'm also not sure it's necessary. Maybe in this case simply deleting the test case is the right thing to do?

Another option: salvage the intent and move the test to PhaseOrdering as a -O1 test?

rampitec added inline comments.Mar 23 2021, 10:05 AM
llvm/lib/Transforms/Scalar/SROA.cpp
1209

Typo: blocl.

lebedev.ri marked an inline comment as done.

Fix typo.

I'm not really opposed to this change, but I'm also not sure it's necessary. Maybe in this case simply deleting the test case is the right thing to do?

Another option: salvage the intent and move the test to PhaseOrdering as a -O1 test?

Sure, i can do that.

lebedev.ri abandoned this revision.Jan 17 2022, 2:38 PM