langref updates for aarch64 trampoline #139740
Conversation
Add clarifying comments to the langref from the review of llvm#126743 (split from the functional changes, to follow).
|
@llvm/pr-subscribers-llvm-ir Author: Jameson Nash (vtjnash) ChangesAdd clarifying comments to the langref from the review of #126743 (split from the functional changes, to follow). @efriedma-quic as discussed here #126743 (comment) Full diff: https://github.com/llvm/llvm-project/pull/139740.diff 1 Files Affected:
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index 5f14726c36672..da1824ba45175 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -410,8 +410,9 @@ added in the future:
calling convention: on most platforms, they are not preserved and need to
be saved by the caller, but on Windows, xmm6-xmm15 are preserved.
- - On AArch64 the callee preserve all general purpose registers, except X0-X8
- and X16-X18.
+ - On AArch64 the callee preserve all general purpose registers, except
+ X0-X8 and X16-X18. Using this calling convention with nest is forbidden
+ and may crash llvm.
The idea behind this convention is to support calls to runtime functions
that have a hot path and a cold path. The hot path is usually a small piece
@@ -447,9 +448,10 @@ added in the future:
R11. R11 can be used as a scratch register. Furthermore it also preserves
all floating-point registers (XMMs/YMMs).
- - On AArch64 the callee preserve all general purpose registers, except X0-X8
- and X16-X18. Furthermore it also preserves lower 128 bits of V8-V31 SIMD -
- floating point registers.
+ - On AArch64 the callee preserve all general purpose registers, except
+ X0-X8 and X16-X18. Furthermore it also preserves lower 128 bits of V8-V31
+ SIMD floating point registers. Using this calling convention with nest is
+ forbidden and may crash llvm.
The idea behind this convention is to support calls to runtime functions
that don't need to call out to any other functions.
@@ -21120,7 +21122,12 @@ sufficiently aligned block of memory; this memory is written to by the
intrinsic. Note that the size and the alignment are target-specific -
LLVM currently provides no portable way of determining them, so a
front-end that generates this intrinsic needs to have some
-target-specific knowledge. The ``func`` argument must hold a function.
+target-specific knowledge.
+
+The ``func`` argument must be a constant (potentially bitcasted) pointer to a
+function declaration or definition, since the calling convention may affect the
+content of the trampoline that is created.
+
Semantics:
""""""""""
|
| and X16-X18. | ||
| - On AArch64 the callee preserve all general purpose registers, except | ||
| X0-X8 and X16-X18. Using this calling convention with nest is forbidden | ||
| and may crash llvm. |
There was a problem hiding this comment.
Please don't put "may crash llvm". That's not contractual: it's a bug if we crash instead of printing a proper error message.
There was a problem hiding this comment.
I'd also consider it a bug and a crash if LLVM prints any error messages (llvm_unreachable is supposed to be, um, unreachable in code that passes the verifier, but it is used for trampoline in backends such as X86ISelLowering with the message "Unsupported calling convention"). I guess it is instead currently just an undocumented behavior that calling conventions are allowed to print errors if the backend doesn't support it?
There was a problem hiding this comment.
We don't usually add a "not all backends support this construct" disclaimers to LangRef; it would pop up in a lot of places, and it wouldn't really be helpful in most of those places.
There is a way to print a proper diagnostic from the backend without crashing, we just don't use it in all the places we should.
There was a problem hiding this comment.
Okay, I see LLVMContext->emitError now (which defaults to calling exit(1), but which can be customized). I guess that existence could potentially be better communicated to frontends? I find it curious that the entire section on how to handle errors in llvm makes no reference of this: https://llvm.org/docs/ProgrammersManual.html#recoverable-errors. And a recently discourse thread on how to handle errors in the backend also claimed it wasn't possible because the proper way to handle it without crashing doesn't exist: https://discourse.llvm.org/t/rfc-report-fatal-error-and-the-default-value-of-gencrashdialog/73587
Would you be willing to review and merge a PR to that section of the manual if I were to add a mention there?
(and perhaps we should also delete the exit(1) in diagnose, instead providing that behavior as an option, so that backends need to properly test that function not returning, and that front end tools are forced to actually implement it better using whatever error recovery–or exit(1) call–that they have)
There was a problem hiding this comment.
I assumed everyone was aware of LLVMContext::diagnose... I did mention it in the discussion (although not by name).
For documentation, that doesn't seem like it's quite the right section of the manual; it's describing generic "error handling", not the rules specific to LLVM passes. We probably should have a description somewhere, though.
I think the reason diagnose() exits by default is so you don't accidentally forget to write a handler, and then continue processing bogus output. In practice, you almost always want one.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/185/builds/18446 Here is the relevant piece of the build log for the reference |
Add clarifying comments to the langref from the review of #126743 (split from the functional changes, to follow).
@efriedma-quic as discussed here #126743 (comment)