[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:788
+          auto *Op = R.getOperand(0);
+          assert(ResSizeInBits > NewResSizeInBits && "Nothing to shrink?");
+          if (GetSizeInBits(Op) >= NewResSizeInBits)
----------------
nit: can assert ResSizeInBits > NewResSizeInBits above, after early-continuing if they're equal.
Actually, they shouldn't even be equal (also compares?), assuming MinBWs is up-to-date and each insn is visited and optimized once. Current code also early-continues when equal, so replacing it with an assert can be done in a separate patch.


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

Yes, the extend-to-Res is replaced by a narrower extend-to-NewRes, but w/o another extend-back-to-Res to provide the original width, might it feed a user, say, a binary operation with mismatched size operands - where the other operand can also shrink to NewRes (as guaranteed by MinBWs) but was extended-back-to-Res? I.e., should all shrunks extend-back-to-Res, or none of them? May need better test coverage.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:804
+        if (GetSizeInBits(Op) == NewResSizeInBits)
+          continue;
+        auto *Shrunk = new VPWidenCastRecipe(
----------------
fhahn wrote:
> 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.
Current code seems to handle selects, and compares, as well as loads and phi's - extending only their result - although MinBWs seems to exclude them(?). So Blend and WidenMemory recipes need not be considered, neither should Replicate recipe - those are to retain their current BW (hence all should extend back to ResTy rather than shrinking all to NewResTy). Worth trying to check if all insns of MinBWs were considered somehow?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.h:90
+  /// Insert truncates and extends for any truncated instructions as hints to
+  /// InstCombine.
+  static void
----------------
fhahn wrote:
> Ayal wrote:
> > Note: a VPlan-based InstCombine could take care of these "hints" by folding redundant extend-truncate pairs.
> Agreed, I think we already have a few separate transforms that could fit into a general instcombine transform
The dead casts removal at the end of current truncateToMinimalBitwidths() should already be taken care of by recipe dce, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149903