This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add loop codegen options to some LLVM dialect ops.
ClosedPublic

Authored by arpith-jacob on Feb 16 2021, 3:45 PM.

Details

Summary

Add a Loop Option attribute and generate llvm metadata attached to
branch instructions to control code generation.

Diff Detail

Event Timeline

arpith-jacob created this revision.Feb 16 2021, 3:45 PM
arpith-jacob requested review of this revision.Feb 16 2021, 3:45 PM
mehdi_amini added a comment.EditedFeb 16 2021, 8:48 PM

Thanks for the contribution :)

Why are you modeling this with a string indirection to a dictionary attribute on the module itself instead of adding the attribute directly on every individual operations?

Why are you modeling this with a string indirection to a dictionary attribute on the module itself instead of adding the attribute directly on every individual operations?

The *same* loop options metadata are referenced by multiple branch instructions. When translating to llvm-ir, we need to generate the loop metadata once and reference it within branch instructions. Having these loop options on a common parent-op works well in this case.

Why are you modeling this with a string indirection to a dictionary attribute on the module itself instead of adding the attribute directly on every individual operations?

The *same* loop options metadata are referenced by multiple branch instructions. When translating to llvm-ir, we need to generate the loop metadata once and reference it within branch instructions.

Why isn't falling off naturally from attribute uniquing in the context? You'd get a unique pointer comparison here.

Having these loop options on a common parent-op works well in this case.

Yes but this indirection through string tagging can become out of sync and isn't really robust from a modeling point of view, while this seems like you only introduced it as a convenience for the translation.

(For example: imagine linking two modules and how it would require ad-hoc process for these metadata right now)

ftynse accepted this revision.Feb 17 2021, 10:20 AM

This LGTM with a bit more documentation and comments addressed.

Regarding the indirection, there are multiple operations that are associated with a loop and this modeling specifies exactly this association. The attribute on these operations is exactly a uniqued StringAttr that specifies which loops they are associated with. The loop may or may not additionally say that a loop has properties translatable to LLVM IR metadata. This is specified once per loop. Duplicating those properties in each occurrence of the attribute (that is, on each operation) will much more likely lead to divergent/incompatible properties associated with the same loop, so I agree with the current modeling. Dealing with name conflicts sounds like exactly the job of a module merger, it will already have to do that, e.g., for symbols with private visibility.

mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h
75–88

All these could youse some documentation.

mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h
215

Nit: MLIR uses /// for function- and class-level comments.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
2170

Use .isa instead of dyn_cast if you don't need the result.

2354

Nit: we tend to prefer assert(condition && "some meaningful message") for clarity

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
627

This needs documentation.

658–659

This looks like it can fail silently, without ever reporting something to the user. The code seems to have a verifier for the attribute being an ArrayAttr, so I'd just assert here instead or had used a cast instead of dyn_cast to trigger as assertion.

663

Nit: SmallVector is reexported into mlir:: namespace so you can omit the namespace. Also, it can now automatically compute the number of stack elements based on the sizeof the element type, prefer that to an explicit number of stack elements unless there is strong reason to pick one.

684

Nit: please spell out the types unless they are too long or clear from context (e.g., there's a .cast on the RHS); here and above.

This revision is now accepted and ready to land.Feb 17 2021, 10:20 AM
ftynse added inline comments.Feb 17 2021, 10:22 AM
mlir/test/Dialect/LLVMIR/roundtrip.mlir
424

Please also add tests for parsing failures (at least for all error messages that can be emitted in parsing)

mehdi_amini requested changes to this revision.Feb 17 2021, 10:39 AM

This LGTM with a bit more documentation and comments addressed.

Regarding the indirection, there are multiple operations that are associated with a loop and this modeling specifies exactly this association.
The attribute on these operations is exactly a uniqued StringAttr that specifies which loops they are associated with.

I'm not convinced that this attribute is intended to uniquely identify a single loop, is it?
Even if it is, then tagging a string attribute as part of the dictionary is orthogonal to the indirection, which I'm still not convinced is a good modeling right now.

The loop may or may not additionally say that a loop has properties translatable to LLVM IR metadata. This is specified once per loop. Duplicating those properties in each occurrence of the attribute (that is, on each operation) will much more likely lead to divergent/incompatible properties associated with the same loop, so I agree with the current modeling. Dealing with name conflicts sounds like exactly the job of a module merger, it will already have to do that, e.g., for symbols with private visibility.

I reiterate my concern with this modeling, I am far from convinced that this is the right things in MLIR right now, so please start an RFC on Discourse.

This revision now requires changes to proceed.Feb 17 2021, 10:39 AM

Duplicating those properties in each occurrence of the attribute (that is, on each operation) will much more likely lead to divergent/incompatible properties associated with the same loop, so I agree with the current modeling. Dealing with name conflicts sounds like exactly the job of a module merger, it will already have to do that, e.g., for symbols with private visibility.

To be more specific, I have a strongly opposed point of view here which I believe comes from how things work in LLVM as well: a module merger shouldn't have to deal with such annotation which can't be a core property of the IR (while symbols are). On the other hand a transformation that manipulate the control flow graph at the LLVM level can be required to manage metadata on branches. So the issue of "divergent/incompatible properties associated with the same loop" is intrinsic to the model, and I don't believe is solved by the indirection (creating the divergence by missing an update to an operation seems just as easy and buggy than keep the metadata in sync but with an incorrect update because you didn't process an operation).

Even if it is, then tagging a string attribute as part of the dictionary is orthogonal to the indirection

I don't understand what you mean here. What is the indirection you are complaining about?

The loop may or may not additionally say that a loop has properties translatable to LLVM IR metadata. This is specified once per loop. Duplicating those properties in each occurrence of the attribute (that is, on each operation) will much more likely lead to divergent/incompatible properties associated with the same loop, so I agree with the current modeling. Dealing with name conflicts sounds like exactly the job of a module merger, it will already have to do that, e.g., for symbols with private visibility.

I reiterate my concern with this modeling, I am far from convinced that this is the right things in MLIR right now, so please start an RFC on Discourse.

What are the alternatives?

Even if it is, then tagging a string attribute as part of the dictionary is orthogonal to the indirection

I don't understand what you mean here. What is the indirection you are complaining about?

The storage of the metadata on the module, which looks to me like a hack to address the lack of support for the LLVM "distinct metadata" in MLIR.

The loop may or may not additionally say that a loop has properties translatable to LLVM IR metadata. This is specified once per loop. Duplicating those properties in each occurrence of the attribute (that is, on each operation) will much more likely lead to divergent/incompatible properties associated with the same loop, so I agree with the current modeling. Dealing with name conflicts sounds like exactly the job of a module merger, it will already have to do that, e.g., for symbols with private visibility.

I reiterate my concern with this modeling, I am far from convinced that this is the right things in MLIR right now, so please start an RFC on Discourse.

What are the alternatives?

I believe we're hitting here the fact that LLVM models this with distinct metadata, which isn't a concept in MLIR. So either we should model things differently in MLIR for concepts that are using a "distinct metadata" or we should provide an equivalent concept to allow this modeling (and I'm not convinced that a DictionnaryAttribute on the module is the best modeling, but we can evaluate this in a Discourse thread, I'm interested to discuss this further with River as well).
This is something that will come up every time we need to handle this kind of metadata, and is worth a solid design.

Addressed comments.

arpith-jacob marked 9 inline comments as done.Feb 18 2021, 6:12 AM

Updated implementation to use an llvm.metadata operation as suggested in
https://llvm.discourse.group/t/annotating-loops-with-codegen-options/2854

ftynse accepted this revision.Mar 3 2021, 1:17 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
861

Nit: we usually add the keyword attributes before attr-dict if there is also a region present.

mlir/lib/Target/LLVMIR/Dialect/LLVMIR/LLVMToLLVMIRTranslation.cpp
245

This function never fails, so let's make it return void

Thanks for the review. Addressed all comments.

mehdi_amini accepted this revision.Mar 3 2021, 10:25 AM

Great, thanks!

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
2205

Nit: be mindful of auto in general, keep it to where it makes the code more readable and/or the type is obvious from the context.

mlir/test/Target/llvmir.mlir
1483

The syntax is quite heavy here, can we make it look something like: options = #llvm.loopopt<disable_unroll = true, disable_licm = true, interleave_count = 1> ?

This revision is now accepted and ready to land.Mar 3 2021, 10:25 AM

Replaced auto with type in some instances.

arpith-jacob marked an inline comment as done.Mar 3 2021, 11:14 AM
arpith-jacob added inline comments.
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
2205

Added the specific type in several places to improve readability.

mlir/test/Target/llvmir.mlir
1483

Each option is a distinct attribute. It seems to me that the mlir infrastructure requires prefixing "llvm." to each llvm dialect specific attribute, thus the "llvm.loopopt" prefix. I'll leave it as is.

mehdi_amini added inline comments.Mar 3 2021, 11:18 AM
mlir/test/Target/llvmir.mlir
1483

I'm sure River has a suggestion here :)

mehdi_amini added inline comments.Mar 3 2021, 1:56 PM
mlir/test/Target/llvmir.mlir
1483

Seems like the most convenient is D97589, but I'm fine landing this patch and migrating it to D97589 afterward!

arpith-jacob marked an inline comment as done.Mar 3 2021, 2:16 PM
arpith-jacob added inline comments.
mlir/test/Target/llvmir.mlir
1483

SG. I'll investigate D97589 once it lands.

Could you please commit this revision? Thank you.

This revision was automatically updated to reflect the committed changes.