Page MenuHomePhabricator

Change the printing/parsing behavior for Attributes used in declarative assembly format
ClosedPublic

Authored by mehdi_amini on Nov 15 2021, 12:34 AM.

Details

Summary

The new form of printing attribute in the declarative assembly is eliding the #dialect.mnemonic prefix to only keep the <....> part.

This is going along with https://llvm.discourse.group/t/rfc-elide-type-attribute-prefixes-when-using-declarative-assembly/4759

Diff Detail

Event Timeline

mehdi_amini created this revision.Nov 15 2021, 12:34 AM
mehdi_amini requested review of this revision.Nov 15 2021, 12:34 AM
mehdi_amini edited the summary of this revision. (Show Details)Nov 15 2021, 12:44 AM
rriddle added inline comments.Nov 15 2021, 12:49 AM
mlir/include/mlir/IR/BuiltinAttributeInterfaces.h
22 ↗(On Diff #387164)

Why is this necessary?

mlir/include/mlir/IR/BuiltinAttributeInterfaces.td
420 ↗(On Diff #387164)

Unrelated?

mlir/include/mlir/IR/OpImplementation.h
85–91

Do we need to publicly expose this? Or can we move this to a protected/private section?

653

Can we drop the llvm:: here?

730–735

Can we drop all of the ::mlir:: here?

767–774

Same comment about dropping ::mlir::.

868

Can we drop the llvm::?

909–913

Same comment about ::mlir::.

mlir/lib/IR/BuiltinAttributes.cpp
65–71

While I think it would be nice to write these in terms of AsmParser/AsmPrinter at some point when we can, do we need to provide any of these given that we can use SFINAE to avoid accidentally calling these?

mlir/test/lib/Dialect/Test/TestTypes.cpp
305–306 ↗(On Diff #387164)

Why do we have a static_assert for an attribute in TestTypes? Also, maybe add a comment here.

mlir/tools/mlir-tblgen/OpFormatGen.cpp
1371–1372

else if? Could also use TypeSwitch with a generic lambda here, but no preference.

2079–2082

Same here.

mehdi_amini added inline comments.Nov 15 2021, 10:26 AM
mlir/lib/IR/BuiltinAttributes.cpp
65–71

If you noticed, I am only providing the implementation: these classes already declare these methods!

So we already use SFINAE, but that leads to a linker error right now.

rriddle added inline comments.Nov 15 2021, 2:02 PM
mlir/lib/IR/BuiltinAttributes.cpp
65–71

Why do they declare these methods? Can we avoid that?

Mogball added a subscriber: Mogball.Dec 2 2021, 2:27 AM
mehdi_amini added inline comments.Dec 4 2021, 2:05 PM
mlir/lib/IR/BuiltinAttributes.cpp
65–71

Right now I don't think we can, this is part of the contract for AttrDef. We could special case the builtin dialect, but frankly I'd rather move toward having less special case (like moving the handling for the builtin stuff outside of the parser instead.

mlir/test/lib/Dialect/Test/TestTypes.cpp
305–306 ↗(On Diff #387164)

That's debugging leftover

mehdi_amini marked 11 inline comments as done.

Address comments

Remove more ::mlir:: qualifiers

rriddle accepted this revision.Dec 7 2021, 10:03 AM
rriddle added inline comments.
mlir/include/mlir/IR/OpImplementation.h
67–68

Can you not use the enable_if in the template? or as the return type? Using it as a last param always kind of makes me feel weird, because it could be provided accidentally.

767–769
910–912
mlir/lib/IR/BuiltinAttributeInterfaces.cpp
12 ↗(On Diff #391882)

Why is this one necessary?

mlir/lib/Parser/AsmParserImpl.h
352–355

Is the top-check necessary? Looks like it's doing the same thing as the else branch here.

363–366

Same comment here.

mlir/tools/mlir-tblgen/OpFormatGen.cpp
2087–2094

Is there a downside to always calling the stripped version? (if it isn't a derived type we will revert to the fallback anyways.

This revision is now accepted and ready to land.Dec 7 2021, 10:03 AM
mehdi_amini marked 8 inline comments as done.Dec 7 2021, 5:20 PM
mehdi_amini added inline comments.
mlir/lib/IR/BuiltinAttributes.cpp
65–71

Jeff's recent patch fixed this: they are all gone now!

mlir/lib/Parser/AsmParserImpl.h
352–355

Not really, it is calling the callback while the else calls the generic parser

mlir/tools/mlir-tblgen/OpFormatGen.cpp
2087–2094

We could call the _odsPrinter.printStrippedAttrOrType in the else branch, but we have an mlir::Type so the SFINAE for printStrippedAttrOrType will resolve to the implementation that is:

template <typename AttrOrType,
    std::enable_if_t<!detect_has_print_method<AttrOrType>::value> *sfinae =
        nullptr>
void printStrippedAttrOrType(
    AttrOrType attrOrType) {
  *this << attrOrType;
}

so inline it here seems more clear to me.

mehdi_amini marked an inline comment as done.

Address comments

Remove unused include

This revision was landed with ongoing or failed builds.Dec 7 2021, 6:02 PM
This revision was automatically updated to reflect the committed changes.