[PATCH] D149903: [VPlan] Replace IR based truncateToMinimalBitwidths with VPlan version.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:749
+    VPlan &Plan, const MapVector<Instruction *, uint64_t> &MinBWs) {
+  auto GetSizeInBits = [](VPValue *Op) {
+    auto *UV = Op->getUnderlyingValue();
----------------
nit: `VPValue *Op` >> `VPValue *VPV`?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:757
+    llvm_unreachable("trying to get type of a VPValue without type info");
+  };
+  for (VPBasicBlock *VPBB : VPBlockUtils::blocksOnly<VPBasicBlock>(
----------------
nit: worth an empty line?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:764-765
+
+      auto *UI =
+          cast_or_null<Instruction>(R.getVPSingleValue()->getUnderlyingValue());
+      auto I = MinBWs.find(UI);
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:766
+          cast_or_null<Instruction>(R.getVPSingleValue()->getUnderlyingValue());
+      auto I = MinBWs.find(UI);
+      if (!UI || I == MinBWs.end())
----------------
nit: is find() ok given a null UI?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:769
+        continue;
+
+      unsigned NewResSizeInBits = I->second;
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:772-773
+      Type *ResTy = UI->getType();
+      if (!ResTy->isIntegerTy() ||
+          ResTy->getScalarSizeInBits() == NewResSizeInBits)
+        continue;
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:787-788
+        case Instruction::ZExt: {
+          assert(ResTy->getScalarSizeInBits() > NewResSizeInBits &&
+                 "Nothing to shrink?");
+          if (GetSizeInBits(R.getOperand(0)) >= NewResSizeInBits)
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:788
+          assert(ResTy->getScalarSizeInBits() > NewResSizeInBits &&
+                 "Nothing to shrink?");
+          if (GetSizeInBits(R.getOperand(0)) >= NewResSizeInBits)
----------------
nit: can set `auto *Op = R.getOperand(0);` for consistency with below.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:790
+          if (GetSizeInBits(R.getOperand(0)) >= NewResSizeInBits)
+            break;
+          auto *C = new VPWidenCastRecipe(cast<CastInst>(UI)->getOpcode(),
----------------
OK, operand < ResTy due to SExt/ZExt,
and NewResTy < ResTy due to MinBW.
NewResTy == ResTy cases should arguably be excluded from MinBWs? (independent of this patch)
Now if operand < NewResTy (< ResTy) then we SExt/ZExt the operand directly to NewResTy instead, and `continue` - why is the "Extend result to original width" part skipped in this case?
If OTOH operand > NewResTy a Trunc is needed rather than an Extend, and provided by subsequent code which is reached by `break`, followed by ZExt back to ResTy.
Otherwise if operand == NewResTy, the SExt/ZExt could be dropped, but we keep it and end up generating a redundant ZExt from R to ResTy - which have same sizes? It's probably ok because the knowledge that NewResTy bits suffice is already there, but would be good to clarify/clean up.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:791
+            break;
+          auto *C = new VPWidenCastRecipe(cast<CastInst>(UI)->getOpcode(),
+                                          R.getOperand(0), NewResTy);
----------------
nit: may look better to take R's opcode than UI's, but that requires casting it to VPWidenCastRecipe, so above isa maybe worth dyn_cast after all...


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:804
+        if (GetSizeInBits(Op) == NewResSizeInBits)
+          continue;
+        auto *Shrunk = new VPWidenCastRecipe(
----------------
assert Op > NewRes? What about the condition operand of select?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:805-806
+          continue;
+        auto *Shrunk = new VPWidenCastRecipe(
+            Instruction::Trunc, Op, IntegerType::get(Ctx, NewResSizeInBits));
+        R.setOperand(Idx, Shrunk);
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:814-815
+      // Extend result to original width.
+      auto *Ext =
+          new VPWidenCastRecipe(Instruction::ZExt, R.getVPSingleValue(), ResTy);
+      ResultVPV->replaceAllUsesWith(Ext);
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:817
+      ResultVPV->replaceAllUsesWith(Ext);
+      Ext->setOperand(0, R.getVPSingleValue());
+      Ext->insertAfter(&R);
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.h:90
+  /// Insert truncates and extends for any truncated instructions as hints to
+  /// InstCombine.
+  static void
----------------
Note: a VPlan-based InstCombine could take care of these "hints" by folding redundant extend-truncate pairs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149903