Page MenuHomePhabricator

[mlir] Start splitting the `tensor` dialect out of `std`.
ClosedPublic

Authored by silvas on Dec 9 2020, 7:05 PM.

Details

Summary

This starts by moving std.extract_element to tensor.extract (this
mirrors the naming of vector.extract).

Curiously, std.extract_element supposedly works on vectors as well,
and this patch removes that functionality. I would tend to do that in
separate patch, but I couldn't find any downstream users relying on
this, and the fact that we have vector.extract made it seem safe
enough to lump in here.

This also sets up the tensor dialect as a dependency of the std
dialect, as some ops that currently live in std depend on
tensor.extract via their canonicalization patterns.

Part of RFC: https://llvm.discourse.group/t/rfc-split-the-tensor-dialect-from-std/2347/2

Diff Detail

Event Timeline

silvas created this revision.Dec 9 2020, 7:05 PM
silvas requested review of this revision.Dec 9 2020, 7:05 PM

Nothing major, but I tend to prefer tensor.get_element to tensor.extract for two reasons: (1) extract in standard data structures/ADT literature also involves removing the element out of the structure (eg. extract_min/extract_max for heaps, priority queues), (2) extract is more suitable for containers but this is an immutable entity.

nicolasvasilache accepted this revision.Dec 9 2020, 11:38 PM

Thanks!

I'm -1 on the proposed name change: LLVM has {insert|extract}{element|value}, op names are globally consistent with that terminology.

This revision is now accepted and ready to land.Dec 9 2020, 11:38 PM
bondhugula added a comment.EditedDec 9 2020, 11:56 PM

Thanks!

I'm -1 on the proposed name change: LLVM has {insert|extract}{element|value}, op names are globally consistent with that terminology.

There are many things MLIR does different from LLVM - there is no need to be consistent with an LLVM naming if that naming is going to be poor for tensor types. There is no extraction happening here - for low-level things like vectors in the LLVM IR, one could make a case for extract though (because you could think of it as being extracted from a vector register).

silvas added a comment.EditedDec 10 2020, 9:39 AM

I generally agree with @bondhugula on tensor.get_element being somewhat better. However, I think tensor.extract is the right thing for this patch because:

  1. The original op used the term "extract", so, despite the inaccurate connotations that @bondhugula points out, this is the status quo and doesn't make things worse.
  2. Consistency with vector.extract (which, as @nicolasvasilache mentions, is also consistent with LLVM's terminology). We should have a joint discussion about renaming both ops, separate from this patch, about that -- in fact, this patch unblocks that discussion from happening in the future!

extract and get_element seem both unambiguous to me in the context we're using them, but I'm biased by coming from LLVM :)
While MLIR is free to diverge, some consistency with LLVM for the same concepts seems valuable to preserve still.

rriddle accepted this revision.Dec 10 2020, 11:34 AM

Thanks Sean! This looks great.

mlir/include/mlir/Dialect/Tensor/IR/TensorBase.td
2

Missing filename here.

45

nit: Remove the extra newline.

mlir/include/mlir/Dialect/Tensor/IR/TensorOps.td
2

Same here.

14

Where is this dependency coming from?

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
157

I'm assuming this is just a staging thing?

silvas updated this revision to Diff 310989.Dec 10 2020, 12:46 PM
silvas marked 2 inline comments as done.

Address River's comments.

mlir/include/mlir/Dialect/Tensor/IR/TensorOps.td
14

Stray copypasta. Removed.

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
157

Canonicalization patterns for std create tensor ops, so we need the dependency, as in any normal case of a dialect's canonicalizations creating ops from another dialect. The idea is that the new tensor dialect is a more fundamental dialect that the old "grab bag of stuff" (i.e. today's std) dialect depends on in the usual fashion.

I believe that once we move enough ops, it can go away though. But the end-goal is of course to have no Standard dialect, so this entire file is staging in a sense.

silvas updated this revision to Diff 311007.Dec 10 2020, 1:29 PM

Remove some more stray copypasta.

silvas updated this revision to Diff 311303.Dec 11 2020, 1:04 PM

Partially revert https://reviews.llvm.org/D92980

As discussed with River (and described in the comment in FoldUtils.cpp), in
the interim state of splitting std, there is a need for the tensor
dialect to materialize constants with std.constant. Implementing the
TensorDialect::materializeConstant hook with std.constant isn't possible
because it would create a cyclic dependency with std.

silvas updated this revision to Diff 311310.Dec 11 2020, 1:44 PM

Touch up a few more places that mention extract_element.

silvas updated this revision to Diff 311311.Dec 11 2020, 1:45 PM

One last nit.

This revision was landed with ongoing or failed builds.Dec 11 2020, 1:58 PM
This revision was automatically updated to reflect the committed changes.