-
Notifications
You must be signed in to change notification settings - Fork 117
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
[CIR][CodeGen] Add initial support for __cxa_rethrow #1290
base: main
Are you sure you want to change the base?
Conversation
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.
Overall this looks good, some minor change needed.
There are quite a number of differences: phi in the CIR version VS loading from %exn.slot in the OG, having multiple landing pads, etc.
Yes, we use block arguments for the slots, and don't unique landing pads (yet). We might stay with the former but fix the later.
The CIR version still seems reasonable to me, mostly because currently we are unable to replicate the exact behavior of the OG codegen. Again, I am very open to more discussions and suggestions here)
I think it's similar enough!
We just went over a rebase against upstream, apologies for the churn but please update your branch for this PR and force-push! |
309a87c
to
39e1a35
Compare
39e1a35
to
dfa69ed
Compare
ba03e3f
to
fa80c8c
Compare
cir::FuncType FTy = | ||
CGF.getBuilder().getFuncType({}, CGF.getBuilder().getVoidTy()); | ||
|
||
auto Fn = CGF.CGM.createRuntimeFunction(FTy, "__cxa_rethrow"); |
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 calling this function, you should emit ThrowOp
here (the version without operands, see rethrows()
defined in the operation) and change LLVM lowering (CIRToLLVMThrowOpLowering::matchAndRewrite
) to handle the rethrows()
version.
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.
To be honest, I think this is fine as it is. A few reasons:
- The OG does it this way
emitRethrow
currently doesn't have any source location information to create aThrowOp
as it is. Of course this is easy to update, I just need to pass theCXXThrowExpr
but I think its worth mentioning.- I am not quite sure how the LLVM lowering will work when we have to lower a
ThrowOp
that is therethrow
version.
For now, I have updated the PR, so we have this structure:
- emitRuntimeFunction
- emitTryCallOp
- emitUnreachable
So, this gets rid of all the basic blocks and ignores all potential code after the throw. I also added llvm output to the tests so you can see.
If you still insist on using the ThrowOp
, it would be nice to get some suggestions on how to implement the LLVM lowering for rethrows in CIRToLLVMThrowOpLowering
.
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.
Thanks for the updates, this looks almost there.
currently doesn't have any source location information to create a ThrowOp as it is
in this case we use assert(CGF.currSrcLoc && "expected source location");
followed by the actual use of CGF.currSrcLoc
.
I am not quite sure how the LLVM lowering will work when we have to lower a ThrowOp that is the rethrow version.
It's actually way easier than the more complex version in CIRToLLVMThrowOpLowering::matchAndRewrite
. Note how we create a declaration for __cxa_throw
and pass down some params. You can identify if the ThrowOp is rethrow by calling op.rethrows(), like explained before. You then create a declaration for __cxa_rethrow
...
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.
You can identify if the ThrowOp is rethrow by calling op.rethrows(), like explained before. You then create a declaration for
__cxa_rethrow
...
Yes, I got this. Okay, I think another thing I forgot to explain is why I used createTryCallOp
. The createTryCallOp
gets flattened and creates a landing pad, because we need the call to __cxa_rethrow
to be an invoke, but if we go through the CIRToLLVMThrowOpLowering
path, how can we make an invoke + landing pad instead of just a call after creating the declaration? It seems currently even emitThrow
has no landing pad logic implemented. TLDR: how do we build a landing pad for the rethrow from CIRToLLVMThrowOpLowering
?
8fffc61
to
2b4a28a
Compare
|
||
mlir::Block *beforeCatch = rewriter.getInsertionBlock(); | ||
rewriter.setInsertionPointToEnd(beforeCatch); | ||
rewriter.replaceOpWithNewOp<cir::BrOp>(tryBodyYield, afterTry); | ||
|
||
if (auto tryBodyYield = dyn_cast<cir::YieldOp>(afterBody->getTerminator())) |
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.
Add a comment here explaining that some other terminator might be present here, e.g. unreachable
cir::FuncType FTy = | ||
CGF.getBuilder().getFuncType({}, CGF.getBuilder().getVoidTy()); | ||
|
||
auto Fn = CGF.CGM.createRuntimeFunction(FTy, "__cxa_rethrow"); |
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.
Thanks for the updates, this looks almost there.
currently doesn't have any source location information to create a ThrowOp as it is
in this case we use assert(CGF.currSrcLoc && "expected source location");
followed by the actual use of CGF.currSrcLoc
.
I am not quite sure how the LLVM lowering will work when we have to lower a ThrowOp that is the rethrow version.
It's actually way easier than the more complex version in CIRToLLVMThrowOpLowering::matchAndRewrite
. Note how we create a declaration for __cxa_throw
and pass down some params. You can identify if the ThrowOp is rethrow by calling op.rethrows(), like explained before. You then create a declaration for __cxa_rethrow
...
This PR adds an initial support for
__cxa_rethrow
, and one test that produces a rethrow.I am very open to suggestions regarding this PR, because I'm still a bit unsure if this replicates the original codegen properly. For example, using the test added, the OG CodeGen produces:
and the proposed CIR equivalent produces:
There are quite a number of differences:
phi
in the CIR version VS loading from%exn.slot
in the OG, having multiple landing pads, etc.The CIR version still seems reasonable to me, mostly because currently we are unable to replicate the exact behavior of the OG codegen. Again, I am very open to more discussions and suggestions here)