[PATCH] D61158: [SimplifyCFG] use fshr instead of shl/lshr/or

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D61158#1480108 <https://reviews.llvm.org/D61158#1480108>, @jmolloy wrote:

> Hi,
>
> Yes, I also have the same concerns as Roman. If we don't canonicalize to fshr in instcombine, then we shouldn't canonicalize to fshr in simplifycfg. They're both canonicalization passes.
>
> Given the testing burden, if you really want to go down this route I would recommend changing instcombine *first*, from which you can observe the fallout with a large testing base. Then change this code, which triggers in many fewer cases.


I agree - the canonicalization should happen in instcombine (and that might make doing it in simplifycfg an academic exercise since we can always count on instcombine to do it).

>>>! In D61158 <https://reviews.llvm.org/D61158>, @nikic wrote:
>  As far as I know we don't have any known issues in funnel shift optimization or codegen.

That is my understanding too. Targets that support some kind of rotate/funnel instruction should produce those from the intrinsic. Targets that don't have that should translate exactly to the sh/sh/or sequence when building SDAG. We already canonicalize to the funnel shift intrinsics for a variable shift amount in several cases, but nobody bothered to add the constant shift amount pattern because it wasn't causing problems like the variable shift case. But using the intrinsics will still theoretically improve passes like inlining and vectorization (assuming they have their cost models straight).

Final note: please don't look at AggressiveInstCombine as your first source for canonicalization truth; use InstCombine for that and propose changes there. "AIC" is a home for expensive/unusual patterns and (at least currently) doesn't run without -O3.


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

https://reviews.llvm.org/D61158