[PATCH] D147783: [VPlan] Add stride->constant VPlan mapping at construction.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 4 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8914
+    else
+      Plan->addVPValue(Stride, new VPValue(CI));
+  }
----------------
peixin wrote:
> Ayal wrote:
> > nit: can alternatively do
> > 
> > ```
> >   Plan->getOrAddVPValue(Stride);
> >   assert(Plan->getVPValue(Stride)->getLiveInIRValue() == CI &&
> >          "Adding different constants for the same stride!");
> > ```
> > ?
> I am thinking if it is really needed? The symblic strides are stored in one ValueToValueMap. It seems to be impossible to may different constants for the same stride.
We need to map `Stride -> VPValue(CI)` so if we want to use `getOrAddVPValue` we would need to add a variant that takes an IR value and a VPValue.

> I am thinking if it is really needed? The symblic strides are stored in one ValueToValueMap. It seems to be impossible to may different constants for the same stride.

The map just stores the IR value that's versioned, but doesn't contain the value it is versioned to (this is only in PSE). At the moment, we only version strides to `1`, but in the future this may change and the assert may help to catch inconsistencies.



================
Comment at: llvm/test/Transforms/LoopVectorize/RISCV/strided-accesses.ll:304
 ; CHECK-NEXT:    [[INDEX:%.*]] = phi i64 [ 0, [[VECTOR_PH]] ], [ [[INDEX_NEXT:%.*]], [[VECTOR_BODY]] ]
 ; CHECK-NEXT:    [[OFFSET_IDX:%.*]] = mul i64 [[INDEX]], [[STRIDE]]
 ; CHECK-NEXT:    [[TMP5:%.*]] = mul i64 0, [[STRIDE]]
----------------
Ayal wrote:
> peixin wrote:
> > This symbolic stride is still here.
> Right, this seems to catch (++IV)*stride cases but miss IV+=stride ones...
This should be handled in the latest version. The issue was the the code handling offsets here was using the `VPExternalDefs` mapping.  D147892 removes that mapping and updates all code to use the `Value2VPValue` mapping, catching this case. 

The end result is better than just replacing `%stride` with `1`, as we can just use the canonical IV.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147783