[PATCH] D76500: WIP: GlobalISel: Lower funnel shifts

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/Utils.h:249
+/// element of a constant G_BUILD_VECTOR/G_BUILD_VECTOR_TRUNC. If AllowUndef is
+/// true, then G_IMPLICIT_DEF elements will pass nullptr to Match.
+bool matchUnaryPredicate(
----------------
Comment about nullptr seems wrong. Match doesn't take any pointer arguments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:4975
+        const ConstantInt *C = getConstantIntVRegVal(R, MRI);
+        return !C || C->getValue().urem(BW) != 0;
+      },
----------------
Should be `C && C->getValue().urem(BW) != 0`.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:5079-5080
+LegalizerHelper::lowerFunnelShift(MachineInstr &MI) {
+  // G_FSHL: (X << (Z % BW)) | (Y >> (BW - (Z % BW)))
+  // G_FSHR: (X << (BW - (Z % BW))) | (Y >> (Z % BW))
+  Register Dst = MI.getOperand(0).getReg();
----------------
Please add some kind of comment to the effect that these expansions are not quite right, cos they might try to shift by BW.


================
Comment at: llvm/lib/CodeGen/GlobalISel/Utils.cpp:750
+  const MachineInstr *Def = getDefIgnoringCopies(Reg, MRI);
+  if (AllowUndefs && Def->getOpcode() == TargetOpcode::G_IMPLICIT_DEF)
+    return true;
----------------
Could return a bit earlier with:
```
if (Def->getOpcode() == TargetOpcode::G_IMPLICIT_DEF)
  return AllowUndefs;
```


================
Comment at: llvm/lib/CodeGen/GlobalISel/Utils.cpp:770
+
+  return true;
+}
----------------
You need this `return true` inside the body of the `if` and a `return false` at the end of the function. Alternatively, return early as soon as we know that Def is **not** G_BUILD_VECTOR*.


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

https://reviews.llvm.org/D76500