[PATCH] D68103: [InstCombine] Simplify shift-by-sext to shift-by-zext

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In D68103#1686130 <https://reviews.llvm.org/D68103#1686130>, @spatel wrote:

> There are a handful of existing transforms in InstCombine that look at users rather than operands, so it's not unprecedented. But I'm not sure if there's another/better way.


The other obvious alternative i know of is putting the non-single-use handling into AggressiveInstCombine.

> One possibility would be to ignore the usual hasOneUse() constraint because we're ok with the trade-off (better analysis despite a potential extra instruction).

Hmm, or that. We are sure no other pass will ever do the opposite transform and replace that `zext` with `sext`?

> You could add the basic fold (with m_OneUse()) as a preliminary and non-controversial step? That would reduce risk and make it clear that whatever extra code that we use is specifically for the multi-use case.

Will split the patch up.

> cc @efriedma in case he has advice.

I will fully understand if the greedy variants aren't good for instcombine proper,
i can put the current-ish vaariant into aggressiveinstcombine; there it won't have O(N^2) problem, too.
It's just that the AIC IIRC only runs in -O3, not even -O2?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68103