Page MenuHomePhabricator

[AArch64] Add new target feature to fuse conditional select
ClosedPublic

Authored by evandro on Jan 22 2018, 12:14 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

evandro created this revision.Jan 22 2018, 12:14 PM
fhahn added a reviewer: fhahn.Jan 23 2018, 2:40 AM
fhahn added inline comments.
llvm/lib/Target/AArch64/AArch64MacroFusion.cpp
158 β†—(On Diff #130943)

Do I understand correctly and are all the changes from the top to here just to avoid returning false early? I think what the existing patterns do is: Either check the first or second opcode. If it's a match, check if the other opcode can be fused with it and return true/false. If the first check is fails, continue. After a quick look, I do not think any of the first checks overlap with the new checks you added in this patch (i.e. no other pattern checks if the SecondOpcode in [ CSELWr, CSELXr ]), so is it really necessary to change the logic above?

On a more general note, it seems like the list of patterns is growing quite large (especially together with D42393). I think going forward it would make it easier if we move the pattern checks in helper functions, where we can just have the switch's and early returns. So in shouldScheduleAdjacent we would just have something like

if (ST.hasFuseAES() && isAESPattern(Op1, Op2)) return true;
if (ST.hasArithmeticCbzFusion() && isCbzFusionPattern(Op1, Op2)) return true;
.....
if (ST.hasFuseCCSelect() && isCCSelectPattern) return true;

IMO this would be easier to read understand and maintain. That's just my personal opinion though, not sure how other people feel.

We could even get rid of all the ifs if we want and just have a single return with all the patterns matching functions we support.

evandro added inline comments.
llvm/lib/Target/AArch64/AArch64MacroFusion.cpp
158 β†—(On Diff #130943)

The change is needed because otherwise the default exit is premature, as other tests may match conditions that failed in one test.

I agree that this is growing unwieldy. Perhaps the time to refactor this code in separate helper functions is nigh. However, I also dislike how hard it is to add new fused pairs. That's why I proposed D35228, which would simplify adding and maintaining fusion quite a bit IMO. Hopefully, I'll be able to round it out as @hfinkel suggested in the coming weeks.

Ping! πŸ””

Β‘Ping! πŸ””πŸ””

Ping!!! πŸ””πŸ””πŸ””

evandro updated this revision to Diff 135539.Feb 22 2018, 3:56 PM

Updated patch considering the refactoring in D43649.

evandro marked 2 inline comments as done.Feb 22 2018, 3:58 PM
evandro updated this revision to Diff 135552.Feb 22 2018, 4:18 PM
fhahn accepted this revision.Feb 22 2018, 8:51 PM

Thanks Evandro! LGTM.

llvm/lib/Target/AArch64/AArch64MacroFusion.cpp
230 β†—(On Diff #135552)

No need for an if here I think, you could just return !AArch64InstrInfo::hasShiftedReg(*FirstMI) ?

234 β†—(On Diff #135552)

Same as above

251 β†—(On Diff #135552)

same as above

256 β†—(On Diff #135552)

same as above

This revision is now accepted and ready to land.Feb 22 2018, 8:51 PM
evandro marked 4 inline comments as done.Feb 23 2018, 8:45 AM
evandro added inline comments.
llvm/lib/Target/AArch64/AArch64MacroFusion.cpp
230 β†—(On Diff #135552)

Of course!

evandro marked 2 inline comments as done.Feb 23 2018, 9:02 AM
This revision was automatically updated to reflect the committed changes.