[CGP] Do not eliminate blocks which have their address taken #163962
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. |
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions cpp -- llvm/lib/CodeGen/CodeGenPrepare.cpp --diff_from_common_commit
View the diff from clang-format here.diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp
index 3b3ab5fc8..34dd7dbf7 100644
--- a/llvm/lib/CodeGen/CodeGenPrepare.cpp
+++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp
@@ -958,7 +958,8 @@ bool CodeGenPrepare::isMergingEmptyBlockProfitable(BasicBlock *BB,
// This could lead to updates across functions which is problematic in a
// function pass like codegenprepare. The update to a blockaddress in a
// function defined before the function with the eliminated block is lost.
- if(BB->hasAddressTaken()) return false;
+ if (BB->hasAddressTaken())
+ return false;
// Do not delete loop preheaders if doing so would create a critical edge.
// Loop preheaders can be good locations to spill registers. If the
|
efriedma-quic
left a comment
efriedma-quic
left a comment
There was a problem hiding this comment.
If a basic block is eliminated before codegen (or before the last IR module pass in the codegen pipeline), there isn't a problem because nothing holds onto the reference.
We have infrastructure for dealing with basic blocks that get eliminated after isel (see AsmPrinter::emitFunctionBody).
And if a block gets eliminated between the start of codegen and isel, we have some code that's supposed to deal with it. See AddrLabelMap::takeDeletedSymbolsForFunction in AsmPrinter.cpp. (see also db035a0). This is supposed to handle testcases like #161164. If that code isn't functioning correctly, we should fix it.
Trying to prevent any codegen IR pass from making code unreachable is way too much work, and impossible to maintain long-term.
Looks like the code in AddrLabelMap in the end just adds a label definition at the beginning of a function, if it was collected previously as a deleted symbol. llvm-project/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp Lines 1058 to 1066 in a4dde44 Deleted symbols are collected in AddrLabelMap::UpdateForDeletedBlock. This sounds like a band aid to not get the undefined symbol error, if we spot the missing location to call UpdateForDeletedBlock. But I have the feeling the code is broken afterwards. We are just placing the symbol somewhere. It does not fix the lost update (neither does my fix of avoiding block elimination). codegenprepare updates the blockaddress referring to the eliminated block, but it does not work if blockaddress is in a function defined before the current one. |
blockaddress constants are specifically designed to be used as inputs to indirectbr. Any other use of a blockaddress constant is outside of the IR model. In particular, if a block has no indirectbr predecessors, we provide no guarantees at all about the value of a blockaddress. So... in cases like #161164, yes, it just needs to point somewhere. |
Ok, makes sense. All indirectbr instructions got eliminated in #161164, therefore the value of blockaddress is unimportant. One can see if a block is targeted by a indirectbr by looking at the terminator in each predecessor. Thank you for explaining. |
When eliminating a block, codegenprepare updates all blockaddress expressions which reference the block. In case the blockaddress is located in a different function, it leads to updates across function boundaries, which is problematic for a function pass like codegenprepare. If blockaddress is in a function defined before the current one, the update to the blockaddress is lost. This change adds a check to avoid eliminations of any block which has its address taken. Fixes: llvm#161164
tetzank
force-pushed
the
cgp-address-taken
branch
from
0d6d6c3 to
c4bb584
Compare
|
@llvm/pr-subscribers-llvm-transforms Author: None (tetzank) ChangesWhen eliminating a block, codegenprepare updates all blockaddress expressions which reference the block. In case the blockaddress is located in a different function, it leads to updates across function boundaries, which is problematic for a function pass like codegenprepare. If blockaddress is in a function defined before the current one, the update to the blockaddress is lost. This change adds a check to avoid eliminations of any block which has its address taken. Fixes: #161164 Full diff: https://github.com/llvm/llvm-project/pull/163962.diff 3 Files Affected:
diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp
index 9e78ec96a2f27e..551dde26ea5302 100644
--- a/llvm/lib/CodeGen/CodeGenPrepare.cpp
+++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp
@@ -954,6 +954,12 @@ bool CodeGenPrepare::eliminateMostlyEmptyBlocks(Function &F) {
bool CodeGenPrepare::isMergingEmptyBlockProfitable(BasicBlock *BB,
BasicBlock *DestBB,
bool isPreheader) {
+ // Do not eliminate blocks which have their address taken.
+ // This could lead to updates across functions which is problematic in a
+ // function pass like codegenprepare. The update to a blockaddress in a
+ // function defined before the function with the eliminated block is lost.
+ if(BB->hasAddressTaken()) return false;
+
// Do not delete loop preheaders if doing so would create a critical edge.
// Loop preheaders can be good locations to spill registers. If the
// preheader is deleted and we create a critical edge, registers may be
diff --git a/llvm/test/Transforms/CodeGenPrepare/X86/blockaddress-reversed.ll b/llvm/test/Transforms/CodeGenPrepare/X86/blockaddress-reversed.ll
new file mode 100644
index 00000000000000..708018fbf5f91c
--- /dev/null
+++ b/llvm/test/Transforms/CodeGenPrepare/X86/blockaddress-reversed.ll
@@ -0,0 +1,53 @@
+; RUN: llc <%s | FileCheck %s
+target triple = "x86_64-unknown-linux-gnu"
+
+; CHECK: .Ltmp0: # Address of block that was removed by CodeGen
+; CHECK-NEXT: .Ltmp1: # Address of block that was removed by CodeGen
+
+define i64 @main(ptr %p, i1 %c1) {
+first:
+ br label %loop_head
+
+loop_head: ; preds = %loop_end, %first
+ br i1 %c1, label %scope0, label %scope1
+
+scope1: ; preds = %loop_head
+ br i1 %c1, label %scope1_exit, label %scope2
+
+scope2: ; preds = %scope1
+ br i1 %c1, label %scope2_exit, label %inner
+
+inner: ; preds = %scope2
+ br label %scope2_exit
+
+scope2_exit: ; preds = %inner, %scope2
+ %phi2 = phi ptr [ %p, %inner ], [ null, %scope2 ]
+ br label %scope1_exit
+
+scope1_exit: ; preds = %scope2_exit, %scope1
+ %phi1 = phi ptr [ %phi2, %scope2_exit ], [ null, %scope1 ]
+ %val1 = load i128, ptr %phi1, align 16
+ br label %loop_end
+
+scope0: ; preds = %loop_head
+ %ptr0 = select i1 %c1, ptr null, ptr %p
+ %val0 = load i128, ptr %ptr0, align 16
+ br label %loop_end
+
+loop_end: ; preds = %scope0, %scope1_exit
+ %storemerge = phi i128 [ %val1, %scope1_exit ], [ %val0, %scope0 ]
+ store i128 %storemerge, ptr %p, align 16
+ br label %loop_head
+}
+
+define void @foo0(ptr %jumpAddr) {
+; CHECK: movq $.Ltmp0, (%rdi)
+ store ptr blockaddress(@main, %scope2_exit), ptr %jumpAddr, align 8
+ ret void
+}
+
+define void @foo1(ptr %jumpAddr) {
+; CHECK: movq $.Ltmp1, (%rdi)
+ store ptr blockaddress(@main, %scope1_exit), ptr %jumpAddr, align 8
+ ret void
+}
diff --git a/llvm/test/Transforms/CodeGenPrepare/X86/blockaddress.ll b/llvm/test/Transforms/CodeGenPrepare/X86/blockaddress.ll
new file mode 100644
index 00000000000000..8066355eca3745
--- /dev/null
+++ b/llvm/test/Transforms/CodeGenPrepare/X86/blockaddress.ll
@@ -0,0 +1,53 @@
+; RUN: llc <%s | FileCheck %s
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @foo0(ptr %jumpAddr) {
+; CHECK: movq $.Ltmp0, (%rdi)
+ store ptr blockaddress(@main, %scope2_exit), ptr %jumpAddr, align 8
+ ret void
+}
+
+define void @foo1(ptr %jumpAddr) {
+; CHECK: movq $.Ltmp1, (%rdi)
+ store ptr blockaddress(@main, %scope1_exit), ptr %jumpAddr, align 8
+ ret void
+}
+
+; CHECK: .Ltmp0: # Address of block that was removed by CodeGen
+; CHECK-NEXT: .Ltmp1: # Address of block that was removed by CodeGen
+
+define i64 @main(ptr %p, i1 %c1) {
+first:
+ br label %loop_head
+
+loop_head: ; preds = %loop_end, %first
+ br i1 %c1, label %scope0, label %scope1
+
+scope1: ; preds = %loop_head
+ br i1 %c1, label %scope1_exit, label %scope2
+
+scope2: ; preds = %scope1
+ br i1 %c1, label %scope2_exit, label %inner
+
+inner: ; preds = %scope2
+ br label %scope2_exit
+
+scope2_exit: ; preds = %inner, %scope2
+ %phi2 = phi ptr [ %p, %inner ], [ null, %scope2 ]
+ br label %scope1_exit
+
+scope1_exit: ; preds = %scope2_exit, %scope1
+ %phi1 = phi ptr [ %phi2, %scope2_exit ], [ null, %scope1 ]
+ %val1 = load i128, ptr %phi1, align 16
+ br label %loop_end
+
+scope0: ; preds = %loop_head
+ %ptr0 = select i1 %c1, ptr null, ptr %p
+ %val0 = load i128, ptr %ptr0, align 16
+ br label %loop_end
+
+loop_end: ; preds = %scope0, %scope1_exit
+ %storemerge = phi i128 [ %val1, %scope1_exit ], [ %val0, %scope0 ]
+ store i128 %storemerge, ptr %p, align 16
+ br label %loop_head
+}
|
|
I just added two IR regression tests. I did not change the "fix". |
3 participants
When eliminating a block, codegenprepare updates all blockaddress expressions which reference the block. In case the blockaddress is located in a different function, it leads to updates across function boundaries, which is problematic for a function pass like codegenprepare.
If blockaddress is in a function defined before the current one, the update to the blockaddress is lost.
This change adds a check to avoid eliminations of any block which has its address taken.
Fixes: #161164