[PATCH] D72808: [mlir] [VectorOps] Lowering of vector.extract/insert_slices to LLVM IR

[Aart Bik via Phabricator via llvm-commits]

aartbik marked an inline comment as done.
aartbik added inline comments.


================
Comment at: mlir/lib/Dialect/VectorOps/VectorOps.cpp:1699
+    if (auto tupleOp =
+            dyn_cast_or_null<TupleOp>(op.getOperand().getDefiningOp())) {
+      rewriter.replaceOp(op, tupleOp.getOperand(op.getIndex()));
----------------
mehdi_amini wrote:
> rriddle wrote:
> > aartbik wrote:
> > > aartbik wrote:
> > > > rriddle wrote:
> > > > > rriddle wrote:
> > > > > > rriddle wrote:
> > > > > > > This should just be in the 'fold' method instead of a canonicalization pattern.
> > > > > > 'fold' should be used whenever possible because it is applicable in many more places, e.g. during dialect conversion and OpBuilder::createOrFold.
> > > > > Also, please split this out into a different revision as this is unrelated.
> > > > I followed the other "canonicalization" patterns in VectorOps. Just for my own understanding, should these strictly speaking have been written as folders too (the name "Folder" seems to imply that)?
> > > Other question, isn't fold for constants only? Here the tuple itself does not need to be constant, the only thing that matters is that get-on-tuple becomes a pass through of the value at the corresponding index?
> > fold has a slightly different contract than canonicalization patterns, and can be used (generally) for the following:
> > 
> > * In-place canonicalization
> > * Folding to an existing SSA value(does not have to be constant)
> > * Folding to an attribute value
> > 
> > So with that being said, if the canonicalization relies on creating new operations then it must use a pattern.
> River: do we have a doc on this? Seems like potentially a good entry for the [FAQ](https://mlir.llvm.org/getting_started/Faq/) otherwise? 
In particular, it was not immediately apparent to me that we *always* call fold(), passing in constants as arguments when available, but also allowing access to the non-constant operands through the regular getters (giving it potentially "two sources of truth"). Now that I have working, it makes sense, but a bit more API doc would have been helpful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72808