Page MenuHomePhabricator

[mlir] Minor fixes after removing types from attributes
ClosedPublic

Authored by dcaballe on Aug 10 2022, 11:06 AM.

Details

Summary

D130092 removed types from attributes. This patch fixes a minor
issues what was exposed when integrating that change in IREE. An
explicit cast is needed so that the template type of makeArrayRef
is automatically deduced.

Diff Detail

Event Timeline

dcaballe created this revision.Aug 10 2022, 11:06 AM
dcaballe requested review of this revision.Aug 10 2022, 11:06 AM
dcaballe added inline comments.Aug 10 2022, 11:08 AM
mlir/include/mlir/Dialect/GPU/IR/GPUOps.td
1249 ↗(On Diff #451570)

Hey @ThomasRaoux, this AllTypesMatch here is not looking into the variadic container and comparing the types in there. It's just comparing the type of the container. Could you please follow up with a fix for this? I couldn't find an existing predicate to do that.

Actually, this is not working when we actually compare the type of multiple variadic operands with each other. For example:

 def SimdLoopOp : OpenMP_Op<"simdloop", [AttrSizedOperandSegments,
                        AllTypesMatch<["lowerBound", "upperBound", "step"]>]> {
let summary = "simd loop construct";
 let description = [{...}];

 // TODO: Add other clauses
 let arguments = (ins Variadic<IntLikeType>:$lowerBound,
            Variadic<IntLikeType>:$upperBound,
            Variadic<IntLikeType>:$step,
            Optional<I1>:$if_expr,
            Confined<OptionalAttr<I64Attr>, [IntPositive]>:$simdlen,
            UnitAttr:$inclusive
    );

I would appreciate some help here.
Thanks!

To provide more context (I talked to Jeff privately), D130092 introduces some changes where getType returns a ShapedType instead of Type. This leads to some problem in ODS, when we use SameTypeOperands or AllTypeMatch and the getType() for the values to compare result in a mix of ShapedType and Type: we use llvm::makeArrayRef(...) and the template type can't be deduced automatically.

In the following example, $lhs.getType() will return Type and $rhs.getType() will return ShapedType:

def CHECK_ExpectEqConstOp :
     Op<CHECK_Dialect, "expect_eq_const", [SameTypeOperands]> {
    let summary = [{Checks that the tensor operand is equal to some constant}];
    let description =  "";

    let arguments = (ins
      AnyTensor:$lhs,
      ElementsAttr:$value
    );

    let hasCanonicalizer = 1;

    let assemblyFormat = "`(` $lhs `,` $value `)` attr-dict `:` type($lhs)";
  }

Then, the generated llvm::makeArrayRef won't compile:

if (!((::llvm::is_splat(::llvm::makeArrayRef({(*this->getODSOperands(0).begin()).getType(), value().getType()})))))
  return emitOpError("failed to verify that all of {lhs, value} have same type");           
return ::mlir::success();
note: candidate template ignored: deduced conflicting types for param
eter 'T' ('mlir::Type' vs. 'mlir::ShapedType')

Perhaps we should revert the changes related to ShapedType getType() in D130092?

@jpienaar @mehdi_amini

Mogball added a comment.EditedAug 10 2022, 11:38 AM

Going forward, there will be no guarantees that getType will need to return Type. FloatAttr::getType should be changed to return FloatType, for example. The whole point is that an attribute with a type can return a more refined subclass if needed.

Mogball added inline comments.Aug 10 2022, 11:44 AM
mlir/include/mlir/IR/OpBase.td
2327

In the absolute worst case, you could do

makeArrayRef<std::conditional_t<std::is_base_of<::mlir::Type, decltype(...)>, Type, decltype(...)>>({...})
2327

But you could just introduce new traits for variadic and non-variadic types. Unfortunately, these traits have some interactions with OpDefinitionsGen.cpp as well

jpienaar added inline comments.Aug 10 2022, 12:12 PM
mlir/include/mlir/IR/OpBase.td
2327

Here we just want to compare the types correct, so casting to Type (in future getType could return many different types, but all Types yes?) seems to enable the comparison that we want here.

dcaballe updated this revision to Diff 451614.Aug 10 2022, 1:19 PM

Use Jeff's suggestion.

Thanks for the suggestions! The last diff should work.

mlir/include/mlir/IR/OpBase.td
2327

That's why I initially thought but we are also using this trait to compare variadic arguments. getType() returns the type of the variadic container (ValueTypeRange?) so we can't convert everything to Type.

Mogball requested changes to this revision.Aug 11 2022, 8:44 AM
Mogball added inline comments.
mlir/include/mlir/Dialect/GPU/IR/GPUOps.td
1250 ↗(On Diff #451614)

unrelated diff

mlir/include/mlir/IR/OpBase.td
2292

This works, but I don't think it's the best solution (even though I suggested it myself). Why don't we move away from llvm::is_splat and just expand to values[0] == values[1] && values[1] == values[2] etc. This way we don't have to rely on the fickle template parameter inference of makeArrayRef and initializer lists.

This revision now requires changes to proceed.Aug 11 2022, 8:44 AM
rriddle added inline comments.Aug 12 2022, 12:38 PM
mlir/include/mlir/IR/OpBase.td
2292

Yeah we should either expand, or cast all of the values to Type; i.e. by wrapping them with ::mlir::Type().

dcaballe updated this revision to Diff 452378.Aug 12 2022, 9:51 PM

Expanded list values
Fix provided by @antiagainst.

Mogball added inline comments.Aug 13 2022, 7:07 AM
mlir/include/mlir/IR/OpBase.td
2294

Can you add braces around the lhs and rhs arguments?

dcaballe updated this revision to Diff 452480.Aug 13 2022, 11:05 PM

Addressed feedback

Mogball accepted this revision.Aug 15 2022, 10:02 AM
Mogball added inline comments.
mlir/include/mlir/IR/OpBase.td
2290–2296

Does this work?

This revision is now accepted and ready to land.Aug 15 2022, 10:02 AM
dcaballe added inline comments.Aug 15 2022, 10:59 AM
mlir/include/mlir/IR/OpBase.td
2290–2296

Unfortunately, it's not. The first == would return a bool that would be compared against a value and so on...

LGTM

mlir/include/mlir/IR/OpBase.td
2290–2296

Right thanks

This revision was landed with ongoing or failed builds.Aug 15 2022, 12:08 PM
This revision was automatically updated to reflect the committed changes.