This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] relax constraints on tensor.cast with pre-rewriting
ClosedPublic

Authored by aartbik on Apr 28 2023, 3:03 PM.

Diff Detail

Event Timeline

aartbik created this revision.Apr 28 2023, 3:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2023, 3:03 PM
aartbik requested review of this revision.Apr 28 2023, 3:03 PM

This CL looks incomplete, since you're not actually using isSameTypesWithoutEncoding anywhere...

mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensor.h
105 ↗(On Diff #518065)

Should be just "Type" not "Types"

wrengr added inline comments.Apr 28 2023, 5:57 PM
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensor.h
105 ↗(On Diff #518065)

hrmm, it looks like that's the name used by lib/Dialect/Tensor/IR/TensorOps.cpp... So I guess consistency is probably better than grammatical correctness...

wrengr added inline comments.Apr 28 2023, 6:00 PM
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensor.h
105 ↗(On Diff #518065)

Actually, now that I've seen that other version of this function, I think it would be better to have this CL change the lib/Dialect/Tensor/IR/TensorOps.cpp version from being static to being exported via include/Dialect/Tensor/IR/TensorOps.h. And then you should use that public version for whatever you needed here

aartbik updated this revision to Diff 518129.Apr 28 2023, 9:36 PM

add missing file

aartbik updated this revision to Diff 518538.May 1 2023, 12:59 PM

addressed comments

wrengr added inline comments.May 1 2023, 1:04 PM
mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
113

Any particular reason why you moved the definition from where the old one was? Just asking for the sake of minimizing the diff

113

(i.e., the line numbers; not the making it public)

wrengr accepted this revision.May 1 2023, 1:12 PM
wrengr added inline comments.
mlir/include/mlir/Dialect/Tensor/IR/Tensor.h
145

I would use "when" here rather than "but"

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorRewriting.cpp
378

Is there any way to have this case emit a warning message to the user? (or "note" or whatever severity-level seems most appropriate)

This revision is now accepted and ready to land.May 1 2023, 1:12 PM
aartbik marked 2 inline comments as done.May 1 2023, 1:18 PM
aartbik added inline comments.
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorRewriting.cpp
378

We actually wanted to add this to verify() proper (and fully reject such cases). But there was also some push back with adding sparse specific checks in tensor dialect. This walks a bit of a middle ground until we come up with a full design that everybody likes followed by full audit of "dense" code.

aartbik updated this revision to Diff 518550.May 1 2023, 1:31 PM
aartbik marked an inline comment as done.

addressed comment

aartbik marked 2 inline comments as done.May 1 2023, 2:56 PM
aartbik added inline comments.
mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
113

I am usually all +1 on minimizing diffs. But here the original static method was in a section marked " Reassociative reshape ops" so I decided to move it up (where the other public methods are defined, and in same order as header file).

The non-sparse part looks good to me! I'll leave the final approval to @wrengr. Thanks!

wrengr added inline comments.May 1 2023, 3:48 PM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorRewriting.cpp
378

I'm fine with leaving this rewriting case here, I was just wondering whether MLIR had some way to emit warnings despite also returning success. That is, since we consider this rewrite to be dubious, it'd be nice to give users some sort of notification whenever it fires. (perhaps only doing so whenever they set the debugging verbosity high enough)

aartbik marked 2 inline comments as done.May 1 2023, 4:03 PM
aartbik added inline comments.
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorRewriting.cpp
378

For sure, I don't believe we have any good precedent for that at the moment. If we get this, we may add it here (or, hopefully one day, just reject the op in dense verify() case ;-)

This revision was automatically updated to reflect the committed changes.
aartbik marked an inline comment as done.
mlir/lib/Dialect/Tensor/IR/TensorOps.cpp