-
Notifications
You must be signed in to change notification settings - Fork 180
[CIR] Add lowering of BlockAddressOp #1909
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
bcardosolopes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few nits and LGTM
| private: | ||
| // Maps a (function name, label name) pair to the corresponding BlockTagOp. | ||
| // Used to resolve CIR LabelOps into their LLVM BlockTagOp. | ||
| llvm::DenseMap<std::pair<llvm::StringRef, llvm::StringRef>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of std::pair<llvm::StringRef, llvm::StringRef>, it might be more MLIR and clean to create an attribute that wraps these two, just like LLVM dialect does. Can you add the attribute? It's fine if you do that in a follow up PR though, just let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’d prefer to handle this in a future PR. Also, what do you think about changing LabelOp to always use this attribute? I think that would be better, because right now I’m looking at IndirectBrOpand with what we have now, I imagine it could look something like this:
cir.blockaddress(@B, "B") -> !cir.ptr<!void> // here we just have a string, not the actual label
^bb2
cir.label "A"
...
^bb3
cir.label "B"
...
^indirectBr(%addr : !cir.ptr<!void>): // this would act like a PHI node for each block address
cir.indirectBr %addr : !cir.ptr<!void> [ ^bb2, ^bb3 ] // we don’t have enough info to get the block of the label(This is just a rough idea for now.)
The problem is that each blockaddress only has a string, so we don’t actually know the block associated with the label. I think with an attribute we could link the label to the block—but I don’t fully understand how that would work yet.
(@andykaylor, I’d appreciate your input on this as welll)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think about changing
LabelOpto always use this attribute?
Sure thing, once you introduce new attributes they should be used wherever it makes sense.
I imagine it could look something like this:
What's the C/C++ code that might lead to this? Note that in LLVM if you don't have the label hints it's undefined behavior (https://llvm.org/docs/LangRef.html#indirectbr-instruction) so I wouldn't worry about imaginary use cases.
The problem is that each blockaddress only has a string, so we don’t actually know the block associated with the label
For the case where this isn't undefined behavior: the new attribute has to be within the same function and part of one of the cir.labels used. (a) you can walk the blocks and find out (the slow options) the cir.labels, or (b) you can book keep the blocks as you go about emitting the "cir.label" instructions. If it's expensive to find the all block sources, we could just use the superset of all the ones we've seen cir.labels in (but I'd like to see a C/C++ source exercising that)
This PR adds lowering of
BlockAddressOp. It uses two maps,blockInfoToTagOpandunresolvedBlockAddressOp, to defer matchingmlir::LLVM::BlockAddressOpto its correspondingmlir::LLVM::BlockTagOpin cases where the matching label has not yet been emitted.If the
BlockTagOpis not emitted, a placeholder valuestd::numeric_limits<uint32_t>::max()is used, which will later be resolved inresolveBlockAddressOp. Support for indirect goto and label differences will be added in a future PR.