[SelectionDAG] Not issue TRAP node if naked function #132147
Conversation
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-llvm-selectiondag Author: None (yonghong-song) ChangesIn [1], Nikita Popov suggested that during lowering 'unreachable' insn should not generate extra code for naked functions, and this applies to all architectures. Note that for naked functions, 'unreachable' insn is necessary in IR since the basic block needs a terminator to end. This patch checked whether a function is naked function or not. If it is a naked function, 'unreachable' insn will not generate ISD::TRAP. I put the RFC in the subject as I am not sure how to find a test which can validate the change in FastISel.cpp. Any advice is welcome. [1] #131731 Full diff: https://github.com/llvm/llvm-project/pull/132147.diff 3 Files Affected:
diff --git a/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp b/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
index 33dd039c4ab2a..1bbb704fc0774 100644
--- a/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
@@ -1862,6 +1862,9 @@ bool FastISel::selectOperator(const User *I, unsigned Opcode) {
return true;
}
+ if (cast<Instruction>(I)->getFunction()->hasFnAttribute(Attribute::Naked))
+ return true;
+
return fastEmit_(MVT::Other, MVT::Other, ISD::TRAP) != 0;
}
return true;
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index 14bb1d943d2d6..6480135a3b929 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -3514,6 +3514,9 @@ void SelectionDAGBuilder::visitUnreachable(const UnreachableInst &I) {
return;
}
+ if (I.getFunction()->hasFnAttribute(Attribute::Naked))
+ return;
+
DAG.setRoot(DAG.getNode(ISD::TRAP, getCurSDLoc(), MVT::Other, DAG.getRoot()));
}
diff --git a/llvm/test/CodeGen/X86/naked-fn-with-unreachable-trap.ll b/llvm/test/CodeGen/X86/naked-fn-with-unreachable-trap.ll
new file mode 100644
index 0000000000000..94274fcb1c160
--- /dev/null
+++ b/llvm/test/CodeGen/X86/naked-fn-with-unreachable-trap.ll
@@ -0,0 +1,11 @@
+; RUN: llc -o - %s -mtriple=x86_64-linux-gnu -trap-unreachable | FileCheck %s
+; RUN: llc -o - %s -mtriple=x86_64-linux-gnu -trap-unreachable -fast-isel | FileCheck %s
+
+define dso_local void @foo() #0 {
+entry:
+ tail call void asm sideeffect "movl 3,%eax", "~{dirflag},~{fpsr},~{flags}"()
+ unreachable
+}
+; CHECK-NOT: ud2
+
+attributes #0 = { naked }
|
yonghong-song
force-pushed
the
naked-func-no-unreachable-isel
branch
from
6b2399b to
63e3e60
Compare
Good point. I missed GlobalISel. I will try to have a common helper for all these three cases. |
yonghong-song
force-pushed
the
naked-func-no-unreachable-isel
branch
from
63e3e60 to
c75fc1b
Compare
yonghong-song
changed the title
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
llvm/include/llvm/IR/Instructions.h
Outdated
| @@ -4497,6 +4497,9 @@ class UnreachableInst : public Instruction { | |||
| return isa<Instruction>(V) && classof(cast<Instruction>(V)); | |||
| } | |||
|
|
|||
| // Whether to do target lowering in SelectionDAG. | |||
| bool allowISelLowering(bool TrapUnreachable, bool NoTrapAfterNoreturn) const; | |||
There was a problem hiding this comment.
This is a really confusing name... why not shouldLowerToTrap(), or something like that?
Otherwise LGTM
There was a problem hiding this comment.
Okay. Will change the func name as suggested.
In [1], Nikita Popov suggested that during lowering 'unreachable' insn should not generate extra code for naked functions, and this applies to all architectures. Note that for naked functions, 'unreachable' insn is necessary in IR since the basic block needs a terminator to end. This patch checked whether a function is naked function or not. If it is a naked function, 'unreachable' insn will not generate ISD::TRAP. [1] llvm#131731
yonghong-song
force-pushed
the
naked-func-no-unreachable-isel
branch
from
c75fc1b to
b395721
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/30/builds/18081 Here is the relevant piece of the build log for the reference |
|
Sorry for the late comment. I don't think we should do this for WebAssembly. See the comments here: llvm-project/llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp Lines 132 to 139 in 58a0c95 So if we change the return type of define dso_local i32 @naked() naked "frame-pointer"="all" {
call void @main()
unreachable
}With this PR, the generated $ llc -mtriple wasm32 naked-fn-with-frame-pointer.ll
$ llvm-mc -triple=wasm32-unknown-unknown naked-fn-with-frame-pointer.s
...
naked-fn-with-frame-pointer.s:12:2: error: type mismatch, expected [i32] but got []
end_function
^
...The reason is, whether the function returns or not, Wasm's validation rule ensures the function's return type matches the type of the value left on the Wasm value stack at the function end (or anytime there is a I think this PR should have an exception for Wasm target. |
I have a couple issues with the current implementation / existence of the TrapOnUnreachable option. First, it's one of the ugly TargetOptions which isn't part of the IR module. In AMDGPUAttributor we try to detect whether any traps are used to enable ABI inputs required for traps on some subtargets, but this option hides them. We can only semi-reliably check for the option there and detect unreachable, as it's not part of the module it depends on consistency of flags between initial optimize and codegen. Plus it assumes later passes will not introduce new unreachables (although if the setting was reliably determinable, we could at least make a conservatively correct check). Second, this bypasses the ordinary ABI handling you would get for a return. For consistency we should really go through something that looks like LowerReturn for unreachable |
Can we just say you're not allowed to do this? Anything other than an inline asm statement in a naked function has dubious semantics. Even on x86, the generated code for the given example is broken.
I don't disagree, but everything you're describing is existing issues. |
Yes, I don't think this is legal code and should really be rejected by the IR verifier. I know that both Rust and Clang essentially limit the contents of naked functions to inline asm. See: llvm-project/clang/lib/Sema/SemaDecl.cpp Lines 16530 to 16554 in 2426ac6 |
In [1], Nikita Popov suggested that during lowering 'unreachable' insn should not generate extra code for naked functions, and this applies to all architectures. Note that for naked functions, 'unreachable' insn is necessary in IR since the basic block needs a terminator to end.
This patch checked whether a function is naked function or not. If it is a naked function, 'unreachable' insn will not generate ISD::TRAP.
[1] #131731