This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][GlobalISel] Legalization of G_ROTL and G_ROTR
ClosedPublic

Authored by matejam on Jul 2 2021, 5:36 AM.

Details

Summary

Add implementation for the legalization of G_ROTL and G_ROTR machine instructions.
They are very similar to funnel shift instructions, the only difference is funnel shifts have 3 operands,
whereas rotate instructions have two operands, the first being the register that is being rotated and
the second being the number of shifts. The legalization of G_ROTL/G_ROTR is just lowering them into
funnel shift instructions if they are legal.

Diff Detail

Event Timeline

matejam created this revision.Jul 2 2021, 5:36 AM
matejam requested review of this revision.Jul 2 2021, 5:36 AM
arsenm added inline comments.Jul 2 2021, 6:49 AM
llvm/lib/Target/AMDGPU/VOP3Instructions.td
346 ↗(On Diff #356157)

Isn't there a lowering to turn this into fshr? Why do we need to directly select this?

llvm/test/CodeGen/AMDGPU/GlobalISel/rotate_pre_instselect.mir
6 ↗(On Diff #356157)

Don't need the IR section, registers section or most of the MIR extras here

llvm/test/CodeGen/AMDGPU/GlobalISel/rotate_pre_regbankselect.mir
713 ↗(On Diff #356157)

You can simplify out stuff like this, for the purpose of this test it doesn't need the readlanes

matejam updated this revision to Diff 356165.Jul 2 2021, 7:16 AM

Changes in tests.

foad added inline comments.Jul 2 2021, 8:25 AM
llvm/lib/Target/AMDGPU/VOP3Instructions.td
346 ↗(On Diff #356157)

I have previously discussed this with Mateja. There's a combine that turns funnel shifts into rotates, so I wasn't sure if that meant that the rotates were canonical and we had to handle them.

Thinking about it now, I guess we can say that rotates are illegal, and the combine should bail out in that case?

matejam added inline comments.Jul 7 2021, 6:37 AM
llvm/lib/Target/AMDGPU/VOP3Instructions.td
346 ↗(On Diff #356157)

If we disabled the "funnel shift to rotate" combiner with a check if the rotate is legal or not, we would also be disabling that combiner for AArch64, specifically for aarch64-prelegalizer-combiner pass.

foad added inline comments.Jul 7 2021, 6:52 AM
llvm/lib/Target/AMDGPU/VOP3Instructions.td
346 ↗(On Diff #356157)

That combine already checks "is legal or before legalizer", so it should be OK. (It is a bit inefficient, because the pre-legalizer combiner will convert funnel shift -> rotate and then the legalizer will convert it back again, but I think that is OK.)

matejam updated this revision to Diff 362058.Jul 27 2021, 9:10 AM
matejam retitled this revision from [AMDGPU][GlobalISel] Legalization and selection of G_ROTL and G_ROTR to [AMDGPU][GlobalISel] Legalization of G_ROTL and G_ROTR.
matejam edited the summary of this revision. (Show Details)
foad added inline comments.Jul 27 2021, 9:17 AM
llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
2762

Can we guarantee that DstTy is a power of two size here? Otherwise just negating the rotate amount does not work.

2764

You could implement B.buildNeg for this, similar to the existing B.buildNot.

matejam updated this revision to Diff 362322.Jul 28 2021, 3:44 AM

When rotating left, use the right negated rotation (negate the number of shifts) only in cases when the destination type size is a power of 2.

matejam updated this revision to Diff 362331.Jul 28 2021, 4:34 AM

Add tests for non power of 2 types (i15, i31...).

foad added inline comments.Jul 28 2021, 9:09 AM
llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
2797

buildFNeg is only for floating point. You need buildNeg, which does not exist yet, but you could implement it.

matejam updated this revision to Diff 363017.Jul 30 2021, 2:57 AM

Implement integer negation buildNeg() and use it instead of buildFNeg().

matejam updated this revision to Diff 363027.Jul 30 2021, 4:00 AM

Formatting and refactoring.

matejam updated this revision to Diff 363073.Jul 30 2021, 7:02 AM

Formatting and refactoring.

arsenm added inline comments.Jul 30 2021, 5:20 PM
llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
2786

There's nothing target specific here, the generic LegalizerHelper can handle this

llvm/lib/Target/AMDGPU/VOP3Instructions.td
346 ↗(On Diff #356157)

Saying anything is canonical in the MIR is a bit fuzzy. I do think the is legal or before legalizer checks aren't ideal now. I think the combiner needs to distinguish between being legal and not having legalization info

llvm/test/CodeGen/AMDGPU/GlobalISel/rotate_pre_regbankselect.mir
516–613 ↗(On Diff #356157)

Don't need all this meta stuff

713 ↗(On Diff #356157)

You should still remove these intrinsic copies

matejam updated this revision to Diff 365716.Aug 11 2021, 4:38 AM
matejam edited the summary of this revision. (Show Details)

Move the legalization of G_ROTL and G_ROTR from AMDGPULegalizerInfo to LegalizerHelper.

matejam updated this revision to Diff 365719.Aug 11 2021, 4:52 AM

Added a few comments and few minor updates.

matejam updated this revision to Diff 365732.Aug 11 2021, 6:08 AM

Formatting.

foad added inline comments.Aug 23 2021, 7:44 AM
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
6087–6088

This doesn't do what you think it does :) It sets IsFShLegal to the whole expression LI.isLegalOrCustom({FShOpc, {DstTy, AmtTy}}) || LI.isLegalOrCustom({RevFsh, {DstTy, AmtTy}})

6090

... so IsFShLegal is always true here.

matejam added inline comments.Aug 24 2021, 5:01 AM
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
6090

Thanks! :)

matejam updated this revision to Diff 368328.Aug 24 2021, 6:27 AM

Minor bug fix.

foad added inline comments.Aug 24 2021, 6:34 AM
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
6092

What if it's not a power of two? In that case, should we fall through to the G_SHL/G_LSHR expansion below?

matejam added inline comments.Aug 24 2021, 6:50 AM
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
6092

Yes, we should... we go into the "if (!IsFShLegal) " statement only if the instruction with the opcode FShOpc isn't legal and the other is. The problem is if the RevFsh instruction is legal and the FShOpc instruction isn't and the type isn't a power of 2, this code will try to build an instruction which ins't legal, instead of falling through to the G_SHL/G_LSHR part.
I'll fix that right away.

matejam updated this revision to Diff 369083.Aug 27 2021, 6:10 AM

In case the funnel shift instruction is not legal and the type is not a power of 2, use the old way of lowering (G_ROTL/G_ROTR -> G_SHL/G_LSHR).

foad accepted this revision.Aug 27 2021, 7:00 AM

LGTM, thanks!

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
6096

Nit: this would be slightly neater:

if (IsFshLegal) {
  return ...
} else if (isPowerOf2_32(EltSizeInBits)) {
  return ...
}
This revision is now accepted and ready to land.Aug 27 2021, 7:00 AM
matejam updated this revision to Diff 369097.Aug 27 2021, 7:36 AM

Refactoring.

foad accepted this revision.Aug 27 2021, 7:38 AM
This revision was landed with ongoing or failed builds.Sep 7 2021, 7:38 AM
This revision was automatically updated to reflect the committed changes.