-
Notifications
You must be signed in to change notification settings - Fork 14.6k
RuntimeLibcalls: Account for Triple default exception handling #147224
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
RuntimeLibcalls: Account for Triple default exception handling #147224
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
@llvm/pr-subscribers-llvm-ir Author: Matt Arsenault (arsenm) ChangesPreviously we were taking the raw TargetOptions exception mode. The interface isn't great, and interprets none to both mean use We also still get the wrong mode in the linker usecase of I'm not really sure how to write a test for this. Full diff: https://github.com/llvm/llvm-project/pull/147224.diff 1 Files Affected:
diff --git a/llvm/include/llvm/IR/RuntimeLibcalls.h b/llvm/include/llvm/IR/RuntimeLibcalls.h
index 50b9791111ce2..9470d22bcad31 100644
--- a/llvm/include/llvm/IR/RuntimeLibcalls.h
+++ b/llvm/include/llvm/IR/RuntimeLibcalls.h
@@ -53,6 +53,14 @@ struct RuntimeLibcallsInfo {
EABI EABIVersion = EABI::Default, StringRef ABIName = "") {
initSoftFloatCmpLibcallPredicates();
initDefaultLibCallImpls();
+
+ // FIXME: The ExceptionModel parameter is to handle the field in
+ // TargetOptions. This interface fails to distinguish the forced disable
+ // case for targets which support exceptions by default. This should
+ // probably be a module flag and removed from TargetOptions.
+ if (ExceptionModel == ExceptionHandling::None)
+ ExceptionModel = TT.getDefaultExceptionHandling();
+
initLibcalls(TT, ExceptionModel, FloatABI, EABIVersion, ABIName);
}
|
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.
reasonable, anyway the right solution is to derive it from the module attributes or such.
I somehow submitted these in the wrong order, #147225 should be first |
dc7ce29
to
a0a9fd6
Compare
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.
LGTM
If you don't already have plans to fix this, could you please file an issue? |
a0a9fd6
to
e5a1556
Compare
Previously we were taking the raw TargetOptions exception mode. This only works correctly for the TargetLowering usage, when the -exception-model flag is explicitly used. The interface isn't great, and interprets none to both mean use target default and unsupported, such that it's not possible to opt-out of exceptions on targets that report a non-none default. We also still get the wrong mode in the linker usecase of RuntimeLibcalls since it doesn't have the TargetMachine. But at least wrongly being the default is an improvement over being unset. I'm not really sure how to write a test for this.
e5a1556
to
4063247
Compare
That's at least 3 steps away. First need to split the lowering decisions from the set of libcalls, and then turn RuntimeLibcalls into an analysis, then replace the TargetOptions field with a module flag |
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/16/builds/22256 Here is the relevant piece of the build log for the reference
|
Previously we were taking the raw TargetOptions exception mode.
This only works correctly for the TargetLowering usage, when
the -exception-model flag is explicitly used.
The interface isn't great, and interprets none to both mean use
target default and unsupported, such that it's not possible to
opt-out of exceptions on targets that report a non-none default.
We also still get the wrong mode in the linker usecase of
RuntimeLibcalls since it doesn't have the TargetMachine. But at
least wrongly being the default is an improvement over being unset.
I'm not really sure how to write a test for this.