Add a Loop Option attribute and generate llvm metadata attached to
branch instructions to control code generation.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 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)
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 | ||
188 | Nit: MLIR uses /// for function- and class-level comments. | |
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp | ||
2249 | Use .isa instead of dyn_cast if you don't need the result. | |
2460 | Nit: we tend to prefer assert(condition && "some meaningful message") for clarity | |
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp | ||
636 | This needs documentation. | |
667–668 | 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. | |
672 | 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. | |
693 | 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. |
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) |
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.
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).
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?
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.
Updated implementation to use an llvm.metadata operation as suggested in
https://llvm.discourse.group/t/annotating-loops-with-codegen-options/2854
Great, thanks!
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp | ||
---|---|---|
2284 | 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> ? |
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp | ||
---|---|---|
2284 | 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. |
mlir/test/Target/llvmir.mlir | ||
---|---|---|
1483 | I'm sure River has a suggestion here :) |
All these could youse some documentation.