[PATCH] D105347: [AMDGPU][GlobalISel] Legalization of G_ROTL and G_ROTR

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:2753
 
+bool AMDGPULegalizerInfo::legalizeRotate(MachineInstr &MI,
+                                         MachineRegisterInfo &MRI,
----------------
There's nothing target specific here, the generic LegalizerHelper can handle this


================
Comment at: llvm/lib/Target/AMDGPU/VOP3Instructions.td:346
+  (rotr i32:$src0, i32:$src1),
+  (V_ALIGNBIT_B32_e64 VGPR_32:$src0, VGPR_32:$src0, VGPR_32:$src1)
+>;
----------------
foad wrote:
> matejam wrote:
> > foad wrote:
> > > arsenm wrote:
> > > > Isn't there a lowering to turn this into fshr? Why do we need to directly select this?
> > > 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?
> > 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.  
> 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.)
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


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/rotate_pre_regbankselect.mir:516-613
+alignment:       1
+exposesReturnsTwice: false
+legalized:       true
+regBankSelected: false
+selected:        false
+failedISel:      false
+tracksRegLiveness: true
----------------
Don't need all this meta stuff


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/rotate_pre_regbankselect.mir:713
+    $sgpr2 = COPY %18(s32)
+    %19:_(s32) = G_INTRINSIC intrinsic(@llvm.amdgcn.readfirstlane), %31(s32)
+    $sgpr3 = COPY %19(s32)
----------------
arsenm wrote:
> You can simplify out stuff like this, for the purpose of this test it doesn't need the readlanes
You should still remove these intrinsic copies


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105347/new/

https://reviews.llvm.org/D105347