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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:280
-  }
-
   bool hasScalarValue(VPValue *Def, VPIteration Instance) {
----------------
Ayal wrote:
> How/Is this removal related?
The last user of this function has been removed in the patch.


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


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:814
+
+  unsigned ProcessedRecipes = 0;
+  for (VPValue *VPV : Plan.getLiveIns()) {
----------------
Ayal wrote:
> Define `ProcessedRecipes` only for debug?
> 
> /// First truncate live-ins that represent relevant Instructions.
Wrapped and added comment, thanks!


================
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);
----------------
Ayal wrote:
> nit: use `LiveInInst` or something similar rather than `UI`?
Renamed, thanks!


================
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())
----------------
Ayal wrote:
> Would ``MinBWs.lookup(UI)` look better? Returning zero clearly indicates unfound.
Updated, thanks!


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


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


================
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());
----------------
Ayal wrote:
> Set once before the loop for all live-ins to be truncated.
hoisted, thanks!


================
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)) {
----------------
Ayal wrote:
> Any order other than depth first would also do, right?
Yes, I think the order doesn't matter here.


================
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())
----------------
Ayal wrote:
> nit: lookup.
Done, thanks!


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:861
+      Type *ResTy = UI->getType();
+      if (!ResTy->isIntegerTy() || ResSizeInBits == NewResSizeInBits)
+        continue;
----------------
Ayal wrote:
> 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?
Turned `isIntegerTy` into assert but retained size check as there entries where the sizes are the same (e.g. for `truncs`).


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:871
+        default:
+          break;
+        case Instruction::SExt:
----------------
Ayal wrote:
> 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?
Thanks, changed to `if`. I don't think Trunc is handled explicitly in the latest version.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:875
+          auto *Op = R.getOperand(0);
+          if (GetSizeInBits(Op) == NewResSizeInBits) {
+            VPC->replaceAllUsesWith(Op);
----------------
Ayal wrote:
> `// SExt/Zext is redundant - stick with its operand.`
> ?
this check has been moved up and is not needed any longer.


================
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);
----------------
Ayal wrote:
> Place assert earlier?
moved up,, thanks!


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:896
+        unsigned OpSizeInBits = GetSizeInBits(Op);
+        if (OpSizeInBits == NewResSizeInBits)
+          continue;
----------------
Ayal wrote:
> This means the size of all operands is equal to NewResSizeInBits, can this be? 
There are cases where a Zext  narrowed earlier is used as operand here, so the tie is already adjusted.


================
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);
+      }
----------------
Ayal wrote:
> nit: keep consistent with above.
Adjusted, thanks!


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149903