This is an archive of the discontinued LLVM Phabricator instance.

[mlir][EDSC] Retire ValueHandle
ClosedPublic

Authored by nicolasvasilache on Apr 22 2020, 8:55 PM.

Details

Summary

The EDSC discussion thread points out that ValueHandle has become an unnecessary level of abstraction since MLIR switch from Value * to Value everywhere.

This revision removes this level of indirection.

Diff Detail

Event Timeline

Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

Thanks for the cleanup! LGTM in principle, I don't know if anyone wants to review this in details...

ftynse accepted this revision.Apr 23 2020, 1:55 AM

Thanks, great clean up, I like the simplification very much and happy to see this moving!

Most my comments are nits or ideas for further simplifications. Some bigger cleanup ideas (for separate commits):

  • all of ArrayRef<Value> can now be replaced with ValueRange;
  • all of ArrayRef<Type> can now be replaced with TypeRange;
  • there is a couple of functions that became trivial and I wonder if they are worth keeping.

I've also seen some changes from ArrayRef<ValueHandle *> to one of ArrayRef<Value *>, SmallVectorImpl<Value> &, MutableArrayRef<Value> and I can't understand the logic behind the individual choices. My suggestion would be to consistently use one specific type (e.g. MutableArrayRef<Value>) eveywhere for consistency.

Thanks for the cleanup! LGTM in principle, I don't know if anyone wants to review this in details...

I did.

mlir/docs/EDSC.md
18–21

Nit: mlir::OpBuilder::create

35–36

Nit: reformat (this is a doc, so not automatic)

mlir/include/mlir/Dialect/Affine/EDSC/Builders.h
28–29

Nit: these are not lbHandles and ubHandles anymore

mlir/include/mlir/Dialect/LoopOps/EDSC/Builders.h
35

Nit these are not handles anymore

mlir/include/mlir/Dialect/StandardOps/EDSC/Intrinsics.h
97

MutableArrayRef instead of ArrayRef<Value *> ? Okay for a separate commit, but you seem to be doing this already in loop builders.

mlir/lib/Dialect/Affine/EDSC/Builders.cpp
87–88

Nit: BinaryHandle isn't a thing anymore. createBinaryOp ? Do we even need now that it's mostly trivial? Okay for a further cleanup.

mlir/lib/Dialect/Linalg/Transforms/LinalgToLoops.cpp
164

I suppose you can drop the Value(fillOp.value()) magic and just do O(ivs) = fillOp now.

Also, consider using a plain "if" rather than a ternary here for readability.

221

nit: MutableArrayRef

325–326

Nit (for a separate clean up, and based on taste): I'd rather make getInputAndOutputIndices return a struct with named fields than a pair of vectors, the types in such fragment are not obvious

mlir/lib/Dialect/LoopOps/EDSC/Builders.cpp
57

Nit: there are not Handles anymore, and how about MutableArrayRef<Value>?

105

No need for Value(...)

109

Not need for Value(...)

mlir/lib/EDSC/Builders.cpp
158

It looks like you could take a MutableArrayRef here instead (the code below assumes vector has enough elements), and ditch the version with ArrayRef<Value *> below.

Also, this calls for an assert(types.size() == args.size())

This revision is now accepted and ready to land.Apr 23 2020, 1:55 AM
nicolasvasilache marked 14 inline comments as done.Apr 23 2020, 7:20 AM

Thanks for the review!

mlir/include/mlir/Dialect/StandardOps/EDSC/Intrinsics.h
97

re MutableArrayRef<T> vs ArrayRef<T*>, both have issues: MutableArrayRef does not work with initializer lists but can be constructed from 2 pointers (begin, end) which leads to fun behavior. OTOH ArrayRef + vectors is clunky (i.e. the retired makeVectorHandlePtrs). For now I added the minimum necessary, we can reevaluate in the future.

nicolasvasilache marked 2 inline comments as done.Apr 23 2020, 7:22 AM
nicolasvasilache added inline comments.
mlir/include/mlir/Dialect/StandardOps/EDSC/Intrinsics.h
97

OTOH it's true the multiple possibilities make it more confusing and the most important, explicitly spelled out, use case (i.e. 1 capture) works with MutableArrayRef.
I just went ahead and updated everything with MutableArrayRef.

Address review.

bondhugula added inline comments.
mlir/lib/Dialect/LoopOps/EDSC/Builders.cpp
87–89

Do we need these SmallVector's? You can change the args for this to ValueRange's actually.