[PATCH] D86530: [InstCombine] PHI-of-extractvalues -> extractvalue-of-PHI, aka invokes are bad

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In D86530#2242350 <https://reviews.llvm.org/D86530#2242350>, @lebedev.ri wrote:

> In D86530#2238580 <https://reviews.llvm.org/D86530#2238580>, @lebedev.ri wrote:
>
>> In D86530#2238463 <https://reviews.llvm.org/D86530#2238463>, @spatel wrote:
>>
>>> In D86530#2238460 <https://reviews.llvm.org/D86530#2238460>, @lebedev.ri wrote:
>>>
>>>> So before i look into `foldAggregateConstructionIntoAggregateReuse()` further,
>>>> what are your thoughts about teaching InstSimplify to do most basic CSE for PHI nodes?
>>>> (purely by comparing that the incoming values match)
>>>
>>> Instruction::isIdenticalTo()?
>>
>> Hm, i guess, although that one is broken QoI-wise for PHI's - it assumes identical incoming block order, which isn't something that should be depended upon.
>>
>>> I see one use of that via visitStoreInst, so there's possible precedent. Start a llvm-dev thread to get other opinions?
>
> Actually, apparently we don't even do PHI CSE in EarlyCSE,
> so i consider my question to be dumb,
> and directly proceeded with the fix, rG6102310d814ad73eab60a88b21dd70874f7a056f <https://reviews.llvm.org/rG6102310d814ad73eab60a88b21dd70874f7a056f>.

Yikes, that could've/should've gone smoother.

But with that done, i've taken some time to analyze next step.
As of right now, in vanilla llvm test-suite + RawSpeed, there are 41 motivational patterns remain
(`resume` of something that isn't either a landingpad or a PHI of landingpads).

All those cases require multiple levels of PHI's,
and in all of those cases at least one `extractvalue` has an extra use,
which goes against the current legality checking for these transforms.

But even if we relax it to "all PHIs must go away, and at least one extractvalue must go away",
while that does finally catch all the remaining motivational cases,
it causes geomean +0.10% compile time increase (regression),
because of, i believe, use-count checking.
D86882 <https://reviews.llvm.org/D86882>, http://llvm-compile-time-tracker.com/compare.php?from=1d01fc100bb5bef5f5eaf92520b2e52f64ee1d6e&to=d07a30a216f640c26c08771e2f7ecba783f5e44e&stat=instructions

So it would appear i will indeed have to enhance `foldAggregateConstructionIntoAggregateReuse()` :/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86530