[PATCH] D89480: [GlobalISel][Legalizer] Implement lower action for G_FSHL/G_FSHR

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:6246-6247
+
+  if (DstTy.isVector())
+    return UnableToLegalize;
+
----------------
Doesn't this lowering work for vectors?


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:6252
+
+  auto Mask = MIRBuilder.buildConstant(DstTy, DstTy.getScalarSizeInBits() - 1);
+  auto BitWidthC = MIRBuilder.buildConstant(DstTy, DstTy.getScalarSizeInBits());
----------------
Is DstTy guaranteed to be the same type as the shift amount operand?


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:6254-6255
+  auto BitWidthC = MIRBuilder.buildConstant(DstTy, DstTy.getScalarSizeInBits());
+  Register ShAmtReg = MRI.createGenericVirtualRegister(DstTy);
+  MIRBuilder.buildInstr(TargetOpcode::G_UREM, {ShAmtReg},
+                        {ZReg, BitWidthC.getReg(0)});
----------------
Can't you just use `auto ShAmt = MIRBuilder.buildWhatever(...)` to avoid explicitly calling createGenericVirtualRegister here and below?


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:6255-6256
+  Register ShAmtReg = MRI.createGenericVirtualRegister(DstTy);
+  MIRBuilder.buildInstr(TargetOpcode::G_UREM, {ShAmtReg},
+                        {ZReg, BitWidthC.getReg(0)});
+
----------------
Do we have something that will combine this UREM into an AND if BW is a power of two?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89480