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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Various nits.



================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:280
-  }
-
   bool hasScalarValue(VPValue *Def, VPIteration Instance) {
----------------
How/Is this removal related?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:805-806
+  auto GetSizeInBits = [](VPValue *VPV) {
+    auto *UV = VPV->getUnderlyingValue();
+    if (UV)
+      return UV->getType()->getScalarSizeInBits();
----------------
nit


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:808-810
+    if (auto *VPC = dyn_cast<VPWidenCastRecipe>(VPV)) {
+      return VPC->getResultType()->getScalarSizeInBits();
+    }
----------------
nit


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:814
+
+  unsigned ProcessedRecipes = 0;
+  for (VPValue *VPV : Plan.getLiveIns()) {
----------------
Define `ProcessedRecipes` only for debug?

/// First truncate live-ins that represent relevant Instructions.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:815
+  unsigned ProcessedRecipes = 0;
+  for (VPValue *VPV : Plan.getLiveIns()) {
+    auto *UI = dyn_cast<Instruction>(VPV->getLiveInIRValue());
----------------
(Future) Thought: wonder if instead of iterating over all live-ins looking to truncate any, it may be better to iterate over MinBWs and check if any are live-ins. Or lookup MinBWs upon construction of a live-in.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:816
+  for (VPValue *VPV : Plan.getLiveIns()) {
+    auto *UI = dyn_cast<Instruction>(VPV->getLiveInIRValue());
+    auto I = MinBWs.find(UI);
----------------
nit: use `LiveInInst` or something similar rather than `UI`?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:817
+    auto *UI = dyn_cast<Instruction>(VPV->getLiveInIRValue());
+    auto I = MinBWs.find(UI);
+    if (!UI || I == MinBWs.end())
----------------
Would ``MinBWs.lookup(UI)` look better? Returning zero clearly indicates unfound.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:822
+    unsigned NewResSizeInBits = I->second;
+    Type *ResTy = VPV->getLiveInIRValue()->getType();
+    if (!ResTy->isIntegerTy())
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:823
+    Type *ResTy = VPV->getLiveInIRValue()->getType();
+    if (!ResTy->isIntegerTy())
+      continue;
----------------
Can this happen - continuing will lose a member of MinBWs - better assert instead?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:829
+    auto *Shrunk = new VPWidenCastRecipe(Instruction::Trunc, VPV, NewResTy);
+    VPBasicBlock *PH = dyn_cast<VPBasicBlock>(
+        Plan.getVectorLoopRegion()->getSinglePredecessor());
----------------
Set once before the loop for all live-ins to be truncated.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:838
+  for (VPBasicBlock *VPBB : VPBlockUtils::blocksOnly<VPBasicBlock>(
+           vp_depth_first_deep(Plan.getEntry()))) {
+    for (VPRecipeBase &R : make_early_inc_range(*VPBB)) {
----------------
Any order other than depth first would also do, right?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:849
+      VPValue *ResultVPV = R.getVPSingleValue();
+      auto *UI = cast_or_null<Instruction>(ResultVPV->getUnderlyingValue());
+      auto I = MinBWs.find(UI);
----------------
(Future) Thought: this is an awkward way of retrieving "the" recipe that corresponds to each member of MinBWs - look through all recipes for those having the desired "underlying" insn. Perhaps better lookup MinBWs upon construction of a recipe for an Instruction.
Or migrate the analysis that builds MinBWs to run on VPlan.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:850
+      auto *UI = cast_or_null<Instruction>(ResultVPV->getUnderlyingValue());
+      auto I = MinBWs.find(UI);
+      if (!UI || I == MinBWs.end())
----------------
nit: lookup.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:855
+      ProcessedRecipes++;
+
+      if (isa<VPReplicateRecipe>(&R))
----------------
Would be good to comment how memory and replicate cases are (not) processed.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:861
+      Type *ResTy = UI->getType();
+      if (!ResTy->isIntegerTy() || ResSizeInBits == NewResSizeInBits)
+        continue;
----------------
Better assert than continue? Here ProcessedRecipes was already bumped, but should all MinBWs members correspond to Integer types, of distinct (smaller) size, whether live-in or not?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:871
+        default:
+          break;
+        case Instruction::SExt:
----------------
This deals only with ZExt/SExt, easier to check directly if Opcode is one or the other?

OTOH, better handle Trunc here as well? Is it handled well below?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:875
+          auto *Op = R.getOperand(0);
+          if (GetSizeInBits(Op) == NewResSizeInBits) {
+            VPC->replaceAllUsesWith(Op);
----------------
`// SExt/Zext is redundant - stick with its operand.`
?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:882
+            Opcode = Instruction::Trunc;
+          assert(ResSizeInBits > NewResSizeInBits && "Nothing to shrink?");
+          auto *C = new VPWidenCastRecipe(Opcode, Op, NewResTy);
----------------
Place assert earlier?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:884-885
+          auto *C = new VPWidenCastRecipe(Opcode, Op, NewResTy);
+          C->insertBefore(&R);
+          ResultVPV->replaceAllUsesWith(C);
+          continue;
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:896
+        unsigned OpSizeInBits = GetSizeInBits(Op);
+        if (OpSizeInBits == NewResSizeInBits)
+          continue;
----------------
This means the size of all operands is equal to NewResSizeInBits, can this be? 


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:900-901
+        auto *Shrunk = new VPWidenCastRecipe(Instruction::Trunc, Op, NewResTy);
+        R.setOperand(Idx, Shrunk);
+        Shrunk->insertBefore(&R);
+      }
----------------
nit: keep consistent with above.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:909-911
+      ResultVPV->replaceAllUsesWith(Ext);
+      Ext->setOperand(0, ResultVPV);
+      Ext->insertAfter(&R);
----------------
nit: keep consistent with above.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.h:72
+  /// Insert truncates and extends for any truncated instructions as hints to
+  /// InstCombine.
+  static void
----------------
nit: a VPlan transform should fold redundant ZExt-Trunc pairs rather than leaving them ("as hints") to `InstCombine`.

Being a public method, which does not need SE, should the caller of optimize() precede its call with a direct call to trunctateToMinimalBitwidth(), rather than pass MinBWs to optimize()?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149903