Page MenuHomePhabricator

[SimplifyCFG] use fshr instead of shl/lshr/or
AbandonedPublic

Authored by shawnl on Apr 25 2019, 4:09 PM.

Details

Reviewers
jmolloy
spatel
Summary

This is patch 3 is a series beginning with D61150

We already try (but fail due to lack of sub op)
to convert this to fshr in AggressiveInstCombine.cpp:92.

Rotate instructions can be a single instructions sometimes,
comparedto three. See
https://bugs.llvm.org/show_bug.cgi?id=37461

Diff Detail

Event Timeline

shawnl created this revision.Apr 25 2019, 4:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2019, 4:09 PM
shawnl updated this revision to Diff 196749.Apr 25 2019, 4:10 PM
shawnl edited the summary of this revision. (Show Details)Apr 25 2019, 4:17 PM
shawnl edited the summary of this revision. (Show Details)
nikic added subscribers: spatel, nikic.

As far as I know we don't have any known issues in funnel shift optimization or codegen. We should check with @spatel though.

The description on the patch is really unhelpful.
It should explain why the change is needed/should happen, not how great the new intrinsic is, how bad it is not to support it...
Also, as usual, please upload all patches with full context.

We already try (but fail due to lack of sub op)
to convert this to fshr in AggressiveInstCombine.cpp:92.

There wouldn't be any sub op here, because these are constants,
and we don't seem to canonicalize this rotate pattern with constants to fshl in instcombine/aggressiveinstcombine.
That is a separate issue, i'm not sure if it's a bug or not, so please file a bug.

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.

jmolloy requested changes to this revision.Apr 26 2019, 1:13 AM
This revision now requires changes to proceed.Apr 26 2019, 1:13 AM

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, @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.

shawnl updated this revision to Diff 197006.Apr 28 2019, 1:23 AM
shawnl edited the summary of this revision. (Show Details)
jmolloy added inline comments.Apr 29 2019, 1:07 AM
lib/Transforms/Utils/SimplifyCFG.cpp
5587

I still don't agree with using fshr here; I've given you my rationale before.

there are no problems, as we can just lower fshr down to what I replaced

http://llvm.org/doxygen/TargetLowering_8cpp_source.html#l04428

jmolloy requested changes to this revision.Apr 30 2019, 5:33 AM

I don't agree; if you're going to add this canonicalization, please do it in InstCombine first.

This revision now requires changes to proceed.Apr 30 2019, 5:33 AM
shawnl abandoned this revision.Apr 30 2019, 7:46 AM