Page MenuHomePhabricator

[AMDGPU][GlobalISel] Legalize G_ABS
ClosedPublic

Authored by mbrkusanin on May 13 2021, 2:38 AM.

Details

Summary

Legalize and select G_ABS so that we can use llvm.abs intrinsic

Diff Detail

Event Timeline

mbrkusanin created this revision.May 13 2021, 2:38 AM
mbrkusanin requested review of this revision.May 13 2021, 2:38 AM
foad added inline comments.May 13 2021, 5:55 AM
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
2020

This has to be a signext, so the wide value gets the same sign as the narrow value. Otherwise the wide G_ABS might behave differently from the narrow G_ABS.

llvm/lib/Target/AMDGPU/VOP3Instructions.td
773 ↗(On Diff #345075)

Don't bother with this pattern, just make G_ABS s16 only legal on subtargets that have 16 bit instructions.

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.abs.ll
11

Typo "sgpr" in lots of function names.

foad added inline comments.May 13 2021, 6:15 AM
llvm/lib/Target/AMDGPU/SOPInstructions.td
1370

No need for a separate pattern for this, you can do it as part of the definition of S_ABS_I32. See S_NOT_I32 for an example.

arsenm added inline comments.May 13 2021, 8:40 AM
llvm/lib/Target/AMDGPU/VOP3Instructions.td
758–760 ↗(On Diff #345075)

Should be expanding the vector case in regbankselect

mbrkusanin marked 5 inline comments as done.
  • Addressed comments
foad added inline comments.May 18 2021, 6:55 AM
llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.abs.ll
14

This should be widened so we get s_sext followed by s_abs.

69

It would be simpler to widen this case to 32 bits, so we get the bfe followed by the usual 32-bit sub, max sequence.

foad added inline comments.May 18 2021, 6:59 AM
llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.abs.ll
43–45

Not really related to your patch, but this sequence needs cleaning up. The s_and is redundant, and the s_cmp just computes the same scc value that s_add computed in the first place.

arsenm added inline comments.May 18 2021, 4:16 PM
llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.abs.ll
43–45

This is probably a consequence of not having a pass to minimize scc/physreg liveranges

  • Widen S16 to S32 for GFX6 + GFX7
mbrkusanin added inline comments.May 20 2021, 2:59 AM
llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.abs.ll
43–45

We have this tracked as a separate issue.

foad added inline comments.May 20 2021, 3:58 AM
llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
986

Merge this with the G_SMIN, G_SMAX, G_UMIN, G_UMAX handling above, which is slightly better because it will widen e.g. s8 to s16 and s24 to s32, instead of lowering them.

1004

Same here, mege it with the slightly better G_SMIN, G_SMAX, G_UMIN, G_UMAX handling above.

arsenm added inline comments.May 20 2021, 5:42 AM
llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp
2337–2339

I would hope that you could call the LegalizerHelper for this part. However I see the lower() implementation hardcodes one path and doesn't provide the helpers for the multiple options. Ideally we could clean that up

  • Merged G_ABS with G_SMIN, G_SMAX, G_UMIN, G_UMAX in LegalizerInfo
foad accepted this revision.May 21 2021, 7:18 AM

LGTM, but maybe try Matt's idea about using LegalizerHelper functions as a follow up?

This revision is now accepted and ready to land.May 21 2021, 7:18 AM
  • Added legalizeABS() as a custom lowering to be used by RegBankSelect.

I don't see a way to add multiple paths to already existing lower() for G_ABS and control which one to use. Not sure if this is the cleaner way of doing it that you had in mind.

mbrkusanin requested review of this revision.May 28 2021, 8:52 AM
mbrkusanin updated this revision to Diff 349552.Jun 3 2021, 7:57 AM
  • Add lowerAbsToMaxSub as a new LegalizerHelper function to be used by AMDGPURegisterBankInfo
  • Move default code for lowering G_ABS to lowerAbs to make it more clear there are two ways of legalizing.

Maybe rename lowerAbs to lowerAbsToAshrAddXor or lowerAbsToAddXor?

foad accepted this revision.Jun 3 2021, 8:49 AM

LGTM.

Maybe rename lowerAbs to lowerAbsToAshrAddXor or lowerAbsToAddXor?

Maybe call them lowerAbsToAddXor and lowerAbsToMaxNeg?

llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp
2333

Too many parentheses.

This revision is now accepted and ready to land.Jun 3 2021, 8:49 AM
mbrkusanin updated this revision to Diff 349573.Jun 3 2021, 9:06 AM
mbrkusanin marked an inline comment as done.
  • Suggested renaming
  • Removed extra parentheses
This revision was landed with ongoing or failed builds.Jun 4 2021, 5:50 AM
This revision was automatically updated to reflect the committed changes.