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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 15 inline comments as done.
fhahn 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();
----------------
Ayal wrote:
> nit: `VPValue *Op` >> `VPValue *VPV`?
Updated, thanks!


================
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>(
----------------
Ayal wrote:
> nit: worth an empty line?
added, thanks!


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


================
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())
----------------
Ayal wrote:
> nit: is find() ok given a null UI?
Yes I think so, the keys are pointers and they shouldn't be dereferenced.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:769
+        continue;
+
+      unsigned NewResSizeInBits = I->second;
----------------
Ayal wrote:
> 
Adjusted, thanks!


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


================
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)
----------------
Ayal wrote:
> 
Updated, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:790
+          if (GetSizeInBits(R.getOperand(0)) >= NewResSizeInBits)
+            break;
+          auto *C = new VPWidenCastRecipe(cast<CastInst>(UI)->getOpcode(),
----------------
Ayal wrote:
> 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.
> 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?

In that case, the original (wider) cast is replaced by a new (narrower) cast and there's no need to truncate.

> 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.

Yep.

> 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.

Yes we would at the moment generate redundant extend/trunc chains, which would indeed be good to clean up. I think we could fold those as follow-up.




================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:791
+            break;
+          auto *C = new VPWidenCastRecipe(cast<CastInst>(UI)->getOpcode(),
+                                          R.getOperand(0), NewResTy);
----------------
Ayal wrote:
> 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...
Updated, thanks!


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

Hmm, select would indeed be handled incorrectly, but I wasn't able to find a suitable test case. Removed VPWidenSelect for now, but will try to come up with a test case. Alternatively could leave select-handling in + assert to surface a test case, if one exists.


================
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);
----------------
Ayal wrote:
> 
Updated, thanks!


================
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);
----------------
Ayal wrote:
> 
Done, thanks!


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:792
+      if (auto *VPW = dyn_cast<VPWidenCastRecipe>(&R)) {
+        Instruction *UI = VPW->getUnderlyingInstr();
+        switch (UI->getOpcode()) {
----------------
Ayal wrote:
> UI is aka UV. Better call it UI from the start, as it's an Instruction* rather than Value*.
Renamed, thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149903