Generic Operation builder does not set up properties

MLIR
1

We use some of the auto-generated “generic” builders with return type inference for some our operations in our code base. They roughly look like this:

void MyOp::build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::ValueRange operands, ::llvm::ArrayRef<::mlir::NamedAttribute> attributes) {
  assert(operands.size() >= 1u && "mismatched number of parameters");
  odsState.addOperands(operands);
  odsState.addAttributes(attributes);

  ::llvm::SmallVector<::mlir::Type, 2> inferredReturnTypes;
  if (::mlir::succeeded(MyOp::inferReturnTypes(odsBuilder.getContext(),
          odsState.location, operands,
          odsState.attributes.getDictionary(odsState.getContext()),
          odsState.getRawProperties(),
          odsState.regions, inferredReturnTypes))) {
    assert(inferredReturnTypes.size() == 1u && "mismatched number of return types");
    odsState.addTypes(inferredReturnTypes);
  } else {
    ::llvm::report_fatal_error("Failed to infer result type(s).");
  }
}

The problem is that at no point the properties are set up. The other builders (the ones where the individual operands are provided) do eventually call

odsState.getOrAddProperties<Properties>();

This poses a problem in our case since we rely on operandSegmentSizes (which is a property) to be correctly set up.

We are currently using the release version of LLVM18.

We are unsure what is going on here and could use some help. Thanks!

2

This seems like a bug. Surprised we haven’t hit it before. Could you file a GitHub issue?

3

Could you check if Comparing llvm:main...jpienaar:piper_export_cl_627834620 · llvm/llvm-project · GitHub works for you?

4

Thank you for the quick reply. Yes, that seems to fix the problem in our case. However, some other builders still don’t initialize the properties. I’m talking about the ones with the following signature:

build(::mlir::OpBuilder &, ::mlir::OperationState &odsState, ::mlir::TypeRange resultTypes, ::mlir::ValueRange operands, ::llvm::ArrayRef<::mlir::NamedAttribute> attributes)
5

Yes same issue (fix should be same code even) - I was looking at the one with type inference as you mentioned it. Good to know that worked for you. Just need to add test later today.

6

Problem ran into: not all ops can have their attributes converted to properties in generated builders: Tensor_ExpandShapeOp basically relies on users only calling custom builders, the custom builders calling the generated ones but doing additional attribute populating. But if one tries to convert attributes to properties in the generated builders then that fails as the expected attributes are not yet injected.

7

I see the problem. Also, does that mean that ExpandShapeOp then has reassociation set in the properties struct AND in the normal attribute list?

Do you think it might be an option to just optionally populate the properties from attributes?

8

I’m currently thinking of how to fix it. The easiest would be to say these don’t get default builders generated and one just provides the builders of interest. It’s a bit of a hammer so would consider alternatives first.

I could also try just scoping down for this instance and then generalizing/cleaning up so that this is avoided.

9

This should be fixed now (the end one resulted in least number of breakages I could figure out).

1 Like