This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Fix some edge cases around 0-element TensorFromElementsOp
ClosedPublic

Authored by silvas on Sep 10 2020, 10:08 PM.

Details

Summary

This introduces a builder for the more general case that supports zero
elements (where the element type can't be inferred from the ValueRange,
since it might be empty).

Also, fix up some cases in ShapeToStandard lowering that hit this. It
happens very easily when dealing with shapes of 0-D tensors.

The SameOperandsAndResultElementType is redundant with the new
TypesMatchWith and prevented having zero elements.

Diff Detail

Event Timeline

silvas created this revision.Sep 10 2020, 10:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 10 2020, 10:08 PM
silvas requested review of this revision.Sep 10 2020, 10:08 PM

The SameOperandsAndResultElementType is redundant with the new

TypesMatchWith and prevented having zero elements.

So is it only conceptually redudant but the verification differs or is it really redundant? And why is zero elements different?

Thanks!

mlir/lib/Conversion/ShapeToStandard/ShapeToStandard.cpp
186

Would it make sense to reuse indexTy (line below) here?

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

nit: Remove //

mlir/test/Conversion/ShapeToStandard/shape-to-standard.mlir
251

nit: duplicate white line

ftynse accepted this revision.Sep 11 2020, 8:47 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
1641–1642

Nit: many operations have the types _before_ the values in their constructors, I'd keep it consistent

This revision is now accepted and ready to land.Sep 11 2020, 8:47 AM

The SameOperandsAndResultElementType is redundant with the new

TypesMatchWith and prevented having zero elements.

So is it only conceptually redudant but the verification differs or is it really redundant? And why is zero elements different?

It is fully redundant, and actually just checks the wrong thing. SameOperandsAndResultElementType assumes >= 1 elements because it gets operands[0] as part of its verification; that's the immediate problem. But SameOperandsAndResultElementType uses getElementTypeOrSelf, so it actually would allow slipping through something like tensor_from_elements %tensor : (tensor<?xf32>) -> tensor<?xf32> (the operands should be f32, not tensor<...f32>) because getElementTypeOrSelf doesn't distinguish tensor<?xf32> and f32.

Our new TypesMatchWith (combined with the ordinary ins/outs checking) is the exact predicate that needs to be checked.

silvas updated this revision to Diff 291271.Sep 11 2020, 10:56 AM
silvas marked 3 inline comments as done.

Address comments.

mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
1641–1642

Done. I had to skip the default builders, because it then conflicts with the default builder that takes the result type.

This revision was landed with ongoing or failed builds.Sep 11 2020, 10:58 AM
This revision was automatically updated to reflect the committed changes.