-
Notifications
You must be signed in to change notification settings - Fork 14.6k
[ARM] support -mlong-calls -fPIC on arm32 #39970 #147313
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
base: main
Are you sure you want to change the base?
Conversation
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
@llvm/pr-subscribers-backend-arm Author: None (jiangxuezhi) Changessupoort -mlong-calls -fPIC using GOT Full diff: https://github.com/llvm/llvm-project/pull/147313.diff 2 Files Affected:
diff --git a/llvm/lib/Target/ARM/ARMISelLowering.cpp b/llvm/lib/Target/ARM/ARMISelLowering.cpp
index 2d73725291d11..b73a53cca84e3 100644
--- a/llvm/lib/Target/ARM/ARMISelLowering.cpp
+++ b/llvm/lib/Target/ARM/ARMISelLowering.cpp
@@ -2815,13 +2815,20 @@ ARMTargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
auto PtrVt = getPointerTy(DAG.getDataLayout());
if (Subtarget->genLongCalls()) {
- assert((!isPositionIndependent() || Subtarget->isTargetWindows()) &&
- "long-calls codegen is not position independent!");
// Handle a global address or an external symbol. If it's not one of
// those, the target's already in a register, so we don't need to do
// anything extra.
if (isa<GlobalAddressSDNode>(Callee)) {
- if (Subtarget->genExecuteOnly()) {
+ if (isPositionIndependent() && !Subtarget->isTargetWindows() &&
+ !Subtarget->genExecuteOnly()) {
+ SDValue G = DAG.getTargetGlobalAddress(
+ GVal, dl, PtrVt, 0, GVal->isDSOLocal() ? 0 : ARMII::MO_GOT);
+ Callee = DAG.getNode(ARMISD::WrapperPIC, dl, PtrVt, G);
+ if (!GVal->isDSOLocal())
+ Callee =
+ DAG.getLoad(PtrVt, dl, DAG.getEntryNode(), Callee,
+ MachinePointerInfo::getGOT(DAG.getMachineFunction()));
+ } else if (Subtarget->genExecuteOnly()) {
if (Subtarget->useMovt())
++NumMovwMovt;
Callee = DAG.getNode(ARMISD::Wrapper, dl, PtrVt,
@@ -2839,7 +2846,8 @@ ARMTargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
PtrVt, dl, DAG.getEntryNode(), Addr,
MachinePointerInfo::getConstantPool(DAG.getMachineFunction()));
}
- } else if (ExternalSymbolSDNode *S=dyn_cast<ExternalSymbolSDNode>(Callee)) {
+ } else if (ExternalSymbolSDNode *S =
+ dyn_cast<ExternalSymbolSDNode>(Callee)) {
const char *Sym = S->getSymbol();
if (Subtarget->genExecuteOnly()) {
diff --git a/llvm/test/CodeGen/ARM/long-calls.ll b/llvm/test/CodeGen/ARM/long-calls.ll
new file mode 100644
index 0000000000000..41763801fddd2
--- /dev/null
+++ b/llvm/test/CodeGen/ARM/long-calls.ll
@@ -0,0 +1,32 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc -mtriple=armv7-unknown-linux-gnueabi -relocation-model pic -mattr=+long-calls -o - %s \
+; RUN: | FileCheck %s
+
+@msg = private unnamed_addr constant [12 x i8] c"hello world\00", align 1
+
+declare i32 @puts(ptr)
+
+define void @test() {
+; CHECK-LABEL: test:
+; CHECK: @ %bb.0: @ %entry
+; CHECK-NEXT: .save {r11, lr}
+; CHECK-NEXT: push {r11, lr}
+; CHECK-NEXT: ldr r0, .LCPI0_0
+; CHECK-NEXT: ldr r1, .LCPI0_1
+; CHECK-NEXT: .LPC0_0:
+; CHECK-NEXT: add r0, pc, r0
+; CHECK-NEXT: .LPC0_1:
+; CHECK-NEXT: ldr r1, [pc, r1]
+; CHECK-NEXT: blx r1
+; CHECK-NEXT: pop {r11, pc}
+; CHECK-NEXT: .p2align 2
+; CHECK-NEXT: @ %bb.1:
+; CHECK-NEXT: .LCPI0_0:
+; CHECK-NEXT: .long .Lmsg-(.LPC0_0+8)
+; CHECK-NEXT: .LCPI0_1:
+; CHECK-NEXT: .Ltmp0:
+; CHECK-NEXT: .long puts(GOT_PREL)-(.LPC0_1+8-.Ltmp0)
+entry:
+ %call = call i32 @puts(ptr @msg)
+ ret void
+}
|
e.g. the code is: int main() the Relocation section is: Relocation section '.rel.ARM.exidx' at offset 0x1a8 contains 1 entry: |
llvm/test/CodeGen/ARM/long-calls.ll
Outdated
@@ -0,0 +1,32 @@ | |||
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5 | |||
; RUN: llc -mtriple=armv7-unknown-linux-gnueabi -relocation-model pic -mattr=+long-calls -o - %s \ | |||
; RUN: | FileCheck %s |
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.
Can this be integrated into llvm/test/CodeGen/ARM/subtarget-features-long-calls.ll?
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.
Thank you for the suggestion. I have moved the new test case to llvm/test/CodeGen/ARM/subtarget-features-long-calls.ll.
Callee = | ||
DAG.getLoad(PtrVt, dl, DAG.getEntryNode(), Callee, | ||
MachinePointerInfo::getGOT(DAG.getMachineFunction())); | ||
} else if (Subtarget->genExecuteOnly()) { |
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.
Does the execute-only codepath work correctly with PIC?
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.
Thank you for the reminder. Considering that XOM is a security feature which prevents the creation of constant pools in executable memory and load from memory, referring to this commit: 52a7dd7 and https://community.arm.com/arm-community-blogs/b/architectures-and-processors-blog/posts/what-is-execute-only-memory-xom, it makes sense to first check Subtarget->genExecuteOnly() before evaluating isPositionIndependent().
I have updated the code accordingly. Thanks again for your feedback.
@@ -2839,7 +2846,8 @@ ARMTargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI, | |||
PtrVt, dl, DAG.getEntryNode(), Addr, | |||
MachinePointerInfo::getConstantPool(DAG.getMachineFunction())); | |||
} | |||
} else if (ExternalSymbolSDNode *S=dyn_cast<ExternalSymbolSDNode>(Callee)) { | |||
} else if (ExternalSymbolSDNode *S = | |||
dyn_cast<ExternalSymbolSDNode>(Callee)) { | |||
const char *Sym = S->getSymbol(); | |||
|
|||
if (Subtarget->genExecuteOnly()) { |
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.
Does the external-symbol codepath work correctly with PIC? (I think you can trigger this with llvm.memset or something like that.)
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.
Thank you for your suggestion. I have attempted to make the external-symbol code path work with PIC.
During this process, I found that using ARMISD::WrapperPIC caused issues in generating correct SDValues for certain symbols. For example, the following error was triggered:
LLVM ERROR: Cannot select: t20: i32 = ARMISD::WrapperPIC TargetExternalSymbol:i32 'memset' [TF=8]
This suggests that using the GOT-based approach for PIC is not suitable in this case.
To address this, I instead used the ARMISD::PIC_ADD node to compute PC-relative addresses, which allows proper handling of external symbols under PIC.
In addition, I have added test cases for llvm.memset in:
llvm-project/llvm/test/CodeGen/ARM/subtarget-features-long-calls.ll
Please help review this change — I'd appreciate your feedback on whether this implementation is appropriate.
@@ -4,6 +4,9 @@ | |||
; RUN: llc -mtriple=thumb-- -mcpu=cortex-a8 -relocation-model=static %s -o - -O0 | FileCheck -check-prefix=NO-OPTION %s | |||
; RUN: llc -mtriple=thumb-- -mcpu=cortex-a8 -relocation-model=static %s -o - -O0 -mattr=+long-calls | FileCheck -check-prefix=LONGCALL %s | |||
; RUN: llc -mtriple=thumb-- -mcpu=cortex-a8 -relocation-model=static %s -o - -O0 -mattr=-long-calls | FileCheck -check-prefix=NO-LONGCALL %s | |||
; RUN: llc -mtriple=arm-linux-gnueabi -mcpu=cortex-a8 -relocation-model=pic %s -o - -O0 -mattr=+long-calls | FileCheck -check-prefix=PIC-O0-LONGCALL %s | |||
; RUN: llc -mtriple=arm-linux-gnueabi -mcpu=cortex-a8 -relocation-model=pic %s -o - -O1 -mattr=+long-calls | FileCheck -check-prefix=PIC-LONGCALL %s | |||
; RUN: llc -mtriple=arm-linux-gnueabi -mcpu=cortex-a8 -relocation-model=pic %s -o - -O2 -mattr=+long-calls | FileCheck -check-prefix=PIC-LONGCALL %s |
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 don't need both -O1 and -O2; -O0 is special in this context because it triggers globalisel/fastisel.
Please add RUN lines for execute-only, so we can see they work.
Please regenerate the CHECK lines with update_llc_test_checks.py, so it's easier to read/update.
; PIC-LONGCALL-LABEL: test_memset: | ||
; PIC-LONGCALL: push {r11, lr} | ||
; PIC-LONGCALL: ldr r3, [[MEMSET_LABEL:.*]] | ||
; PIC-LONGCALL: add r3, pc, r3 |
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.
The -O1 code is wrong. The -O0 code is also wrong.
Look at what we do for something like extern int x; int* f() { return &x; }
. Or look at what we do for __attribute((visibility("hidden"))) extern int x; int* f() { return &x; }
.
You can test this locally with the following command:git-clang-format --diff HEAD~1 HEAD --extensions cpp -- llvm/lib/Target/ARM/ARMISelLowering.cpp View the diff from clang-format here.diff --git a/llvm/lib/Target/ARM/ARMISelLowering.cpp b/llvm/lib/Target/ARM/ARMISelLowering.cpp
index 242bec273..6d3192dff 100644
--- a/llvm/lib/Target/ARM/ARMISelLowering.cpp
+++ b/llvm/lib/Target/ARM/ARMISelLowering.cpp
@@ -2802,9 +2802,9 @@ ARMTargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
++NumMovwMovt;
Callee = DAG.getNode(ARMISD::Wrapper, dl, PtrVt,
DAG.getTargetGlobalAddress(GVal, dl, PtrVt));
- } else if (isPositionIndependent() && !Subtarget->isTargetWindows()){
+ } else if (isPositionIndependent() && !Subtarget->isTargetWindows()) {
SDValue G = DAG.getTargetGlobalAddress(
- GVal, dl, PtrVt, 0, GVal->isDSOLocal() ? 0 : ARMII::MO_GOT);
+ GVal, dl, PtrVt, 0, GVal->isDSOLocal() ? 0 : ARMII::MO_GOT);
Callee = DAG.getNode(ARMISD::WrapperPIC, dl, PtrVt, G);
if (!GVal->isDSOLocal())
Callee =
|
support -mlong-calls -fPIC on arm32
try fixing issuse #39970 and maybe helpful for soving the problem encourted in #142982