Page MenuHomePhabricator

[mlir][ods] Add option to elide attribute printing when the value is the default
AbandonedPublic

Authored by jfurtek on Oct 6 2022, 1:56 PM.

Details

Summary

This diff adds a bit to the tblgen Attr class that causes the tblgenerated
print() function to skip printing a DefaultValuedAttr attribute when the value
is equal to the default.

This feature will reduce the amount of custom printing code that needs to be
written by users a relatively common scenario. As a motivating example, for the
fastmath flags in the LLVMIR dialect, we would prefer to print this:

%0 = llvm.fadd %arg0, %arg1 : f32

instead of this:

%0 = llvm.fadd %arg0, %arg1 {fastmathFlags = #llvm.fastmath<none>} : f32

The (in-progress) fastmath support in the arith dialect will use this
feature, as will the LLVMIR dialect. It is expected that other dialects will
also make use of the fastmath attribute, and these dialects can opt-in for the
same behavior. Adding support to tblgen will allow other attributes to
leverage this capability as well.

Diff Detail

Event Timeline

jfurtek created this revision.Oct 6 2022, 1:56 PM
jfurtek requested review of this revision.Oct 6 2022, 1:56 PM

Haven't looked deeply but this kind of feels weird, why wouldn't we just always elide the default?

jfurtek added a subscriber: Mogball.

Adding @Mogball since this is based heavily on his diff for optional parameters (https://reviews.llvm.org/D133816)

Haven't looked deeply but this kind of feels weird, why wouldn't we just always elide the default?

I'm happy to make that the case - it felt presumptuous for me to assume no one would want to see the value in the IR.

Maybe there are situations where the default is non-obvious, and retaining the attribute in the IR prevents people from having to look up the default.

btw D134993 does something similar, though it's for the custom assembly format syntax. so for the fastmath example you could do something like:

let assemblyFormat = "$lhs `,` $rhs (`fastmath` `(` $fastmathFlags^ `)`)? attr-dict `:` type($res)";

which would print/parse as:

llvm.fadd %arg0, %arg1 fastmath(<fast>) : f32
llvm.fadd %arg0, %arg1 : f32  // same as fastmath(<>)

@rriddle @mehdi_amini Is there any chance we can come to a resolution on this? (I'd like to land the fastmath support - which has a default-valued attribute - and a couple of others have expressed interest in it.)

It seems to me that the options are:

  1. Abandon this diff and do nothing: Operations that want to elide printing default attributes can write custom handlers on a per-operation basis.
  2. Use this diff: a tblgen bit is added on a per-attribute basis. Eliding is "opt-in" so as not to change any existing behavior.
  3. Update this diff to "elide-by-default" instead of "opt-in": as suggested by River.
  4. Leverage optional group anchor behavior as in https://reviews.llvm.org/D134993 when reviewed/landed
rriddle added a comment.EditedOct 12 2022, 12:08 AM

@rriddle @mehdi_amini Is there any chance we can come to a resolution on this? (I'd like to land the fastmath support - which has a default-valued attribute - and a couple of others have expressed interest in it.)

It seems to me that the options are:

  1. Abandon this diff and do nothing: Operations that want to elide printing default attributes can write custom handlers on a per-operation basis.
  2. Use this diff: a tblgen bit is added on a per-attribute basis. Eliding is "opt-in" so as not to change any existing behavior.
  3. Update this diff to "elide-by-default" instead of "opt-in": as suggested by River.
  4. Leverage optional group anchor behavior as in https://reviews.llvm.org/D134993 when reviewed/landed

I don't think this bit should be on attributes at all (so -1 to 2), it's specific to operation formats. My comment was directed at the fact that I think we could/should elide default valued attributes from operation dictionaries by default. Otherwise, 4 seems like an easy way to make progress. IIRC we already switched over to populating default attributes on operation construction anyways.

+1 to operation format rather than attribute specific.

I don't think this bit should be on attributes at all (so -1 to 2),

...

+1 to operation format rather than attribute specific.

Why is this preferable?

If I think in terms of a fastmath attribute, there may eventually be 30 or 40 operations that use the attribute. Some of them may not have a custom assembly format, but if I understand correctly the suggestion here, they will have to now if they don't want to print fastmath<none>.

Conceptually, to me, the idea that fastmath<none> is not "useful" information is a property of the attribute itself. (Operations can override via tablegen attribute class inheritance.)

I think we could/should elide default valued attributes from operation dictionaries by default.

I can change this diff to avoid checking the elidePrintingDefaultValue bit on attributes, but it seems like this would break some tests. Would that be acceptable?

I would like to avoid having to pepper custom assembly on every op that wants to use the fastmath attribute.

jfurtek abandoned this revision.Oct 14 2022, 4:02 PM

Abandoning for now - creating a separate diff that removes the tblgen option bit for default-valued attributes.