[PATCH] D159202: [VPlan] Simplify redundant trunc (zext A) pairs to A.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:821
 
+/// Return the scalar size in bits for \p VPV if possible.
+static unsigned getSizeInBits(VPValue *VPV) {
----------------
Ayal wrote:
> rename getTypeSizeInBits()?
> 
> Worth introducing getType(), possibly as a method of VPValue?
> 
> Callers could then get its scalar size in bits if interested, although in this case the types themselves should arguably be compared rather than their scalar sizes in bits?
I think in general there we need to properly infer the types and not rely on the underlying value, see https://github.com/llvm/llvm-project/pull/69013


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:826
+    return UV->getType()->getScalarSizeInBits();
+  if (auto *VPC = dyn_cast<VPWidenCastRecipe>(VPV)) {
+    return VPC->getResultType()->getScalarSizeInBits();
----------------
Ayal wrote:
> Should this cast case be placed first?
Reordered.


================
Comment at: llvm/test/Transforms/LoopVectorize/if-pred-stores.ll:542
   %tmp4 = zext i8 %tmp3 to i32
   %tmp5 = trunc i32 %tmp4 to i8
   store i8 %tmp5, ptr %tmp2, align 1
----------------
Ayal wrote:
> This "removeRedundantCasts" elimination seems to catch opportunities in the original (unoptimized) code rather than opportunities generated by LV itself, as in Mul-by-1/Add-0. But that may change once bitwidth minimization takes place as VPlan2VPlan transform (D149903)?
Yes, this will be enabled/used by D149903


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159202