This is an archive of the discontinued LLVM Phabricator instance.

[ARM][TTI] Prevents constants in a min(max) or max(min) pattern from being hoisted when in a loop
ClosedPublic

Authored by MeeraN on Sep 10 2020, 8:24 AM.

Details

Summary

Changes TTI function getIntImmCostInst to take an additional Instruction parameter, which enables us to be able to check it is part of a min(max())/max(min()) pattern. We can then mark the constant used as free to prevent it being hoisted so SSAT can still be generated.
Required minor changes in some non-ARM backends to allow for the optional parameter to be included.

Diff Detail

Event Timeline

MeeraN created this revision.Sep 10 2020, 8:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 10 2020, 8:24 AM
MeeraN requested review of this revision.Sep 10 2020, 8:24 AM

The TTI changes here look OK to me. I think it is probably worth trying to make the ARM check a bit more specific though. We are trying to match a SSAT, not any min/max. It's worth making sure that the subtarget will have an SSAT instruction, and that the size of the type we will be saturating to will fit into an i32.

I'm not sure about having to match a more specific min(max(..)) and max(min(..)) pattern too. That will depend on how some examples look if they only have the max, but won't produce a ssat.

MeeraN updated this revision to Diff 292224.Sep 16 2020, 8:01 AM
MeeraN edited the summary of this revision. (Show Details)

Made the checks more specific to min(max()) and max(min()) patterns which should generate SSAT

MeeraN updated this revision to Diff 293120.Sep 21 2020, 3:24 AM

Improved getIntImmCostInst so that the max(min()) case is properly handled by checking the instruction's operand is a min and a select instruction.

Thanks. Looks good.

llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp
328

Can you add something about matching ssat in this comment. You could consider pulling it out into a separate function too.

329

The condition might be better as ((ST->hasV6Ops() && !ST->isThumb()) || ST->isThumb2()), as there are some cpu's (like the cortex-m0) which have V6Ops but not A32 or SSat.

346–347

If this checks for MinSPF == SPF_SMIN first, then in case that is false we needn't look at the minRHS.

I think getUniqueInteger can just be getValue too, if we know it's a ConstantInt?

MeeraN updated this revision to Diff 293170.Sep 21 2020, 7:46 AM
MeeraN retitled this revision from [ARM][TTI] Prevents constants in a min/max pattern from being hoisted when in a loop to [ARM][TTI] Prevents constants in a min(max) or max(min) pattern from being hoisted when in a loop.

Added new helper function isSSATMinMaxPattern which checks whether the instruction being passed in is a valid max instruction that is part of a min(max()) or max(min()) pattern. Simplified getIntImmCostInst down to make a call to this function.

dmgreen accepted this revision.Sep 21 2020, 10:03 AM

Very nice, LGTM.

llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp
297

This can use [&]
(it likely doesn't matter a lot, but can prevent a copy).

This revision is now accepted and ready to land.Sep 21 2020, 10:03 AM
This revision was automatically updated to reflect the committed changes.
MeeraN marked 2 inline comments as done.