[CodeGen] Check BlockAddress users before marking block as taken #174480
Conversation
llvmbot
added
backend:X86
llvm:globalisel
llvm:SelectionDAG
|
@llvm/pr-subscribers-backend-aarch64 @llvm/pr-subscribers-llvm-selectiondag Author: Justin Stitt (JustinStitt) ChangesAfter commit 4109bac ("[IR] Do not store Function inside BlockAddress"), the This was causing some objtool warnings about unreachable instructions 1. Check that BlockAddress actually has users before marking the MBB. If the BlockAddress exists but has no users (probably because they were optimized away), we can skip marking the block. The performance impact should be negligible as we still only check for users after verifying that the block address has its address taken somewhere, which should be false pretty often (I think?!). I regenerated
Nearly all of the triage was done by @nathanchance and @ernsteiswuerfel over at #167774 and ClangBuiltLinux/linux#2130. I used a kernel source that @nathanchance reduced in order to test these dead/orphaned block address placements. Full diff: https://github.com/llvm/llvm-project/pull/174480.diff 3 Files Affected:
diff --git a/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp b/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
index 12552bce3caaa..18a6dd2c15365 100644
--- a/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
@@ -4217,8 +4217,14 @@ bool IRTranslator::runOnMachineFunction(MachineFunction &CurMF) {
MBB = MF->CreateMachineBasicBlock(&BB);
MF->push_back(MBB);
- if (BB.hasAddressTaken())
- MBB->setAddressTakenIRBlock(const_cast<BasicBlock *>(&BB));
+ // Only mark the block if the BlockAddress actually has users. The
+ // hasAddressTaken flag may be stale if the BlockAddress was optimized away
+ // but the constant still exists in the uniquing table.
+ if (BB.hasAddressTaken()) {
+ if (BlockAddress *BA = BlockAddress::lookup(&BB))
+ if (!BA->use_empty())
+ MBB->setAddressTakenIRBlock(const_cast<BasicBlock *>(&BB));
+ }
if (!HasMustTailInVarArgFn)
HasMustTailInVarArgFn = checkForMustTailInVarArgFn(IsVarArg, BB);
diff --git a/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp b/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
index e73743ecbc9fa..74100eeeefcdd 100644
--- a/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
@@ -26,6 +26,7 @@
#include "llvm/CodeGen/TargetSubtargetInfo.h"
#include "llvm/CodeGen/WasmEHFuncInfo.h"
#include "llvm/CodeGen/WinEHFuncInfo.h"
+#include "llvm/IR/Constants.h"
#include "llvm/IR/DataLayout.h"
#include "llvm/IR/DerivedTypes.h"
#include "llvm/IR/Function.h"
@@ -277,8 +278,14 @@ void FunctionLoweringInfo::set(const Function &fn, MachineFunction &mf,
// Transfer the address-taken flag. This is necessary because there could
// be multiple MachineBasicBlocks corresponding to one BasicBlock, and only
// the first one should be marked.
- if (BB.hasAddressTaken())
- MBB->setAddressTakenIRBlock(const_cast<BasicBlock *>(&BB));
+ // Only mark the block if the BlockAddress actually has users. The
+ // hasAddressTaken flag may be stale if the BlockAddress was optimized away
+ // but the constant still exists in the uniquing table.
+ if (BB.hasAddressTaken()) {
+ if (BlockAddress *BA = BlockAddress::lookup(&BB))
+ if (!BA->use_empty())
+ MBB->setAddressTakenIRBlock(const_cast<BasicBlock *>(&BB));
+ }
// Mark landing pad blocks.
if (BB.isEHPad())
diff --git a/llvm/test/CodeGen/X86/speculative-load-hardening-indirect.ll b/llvm/test/CodeGen/X86/speculative-load-hardening-indirect.ll
index fd5085c8c2ac9..2092fc9cd6e00 100644
--- a/llvm/test/CodeGen/X86/speculative-load-hardening-indirect.ll
+++ b/llvm/test/CodeGen/X86/speculative-load-hardening-indirect.ll
@@ -464,34 +464,34 @@ define dso_local i32 @test_indirectbr_global(i32 %idx) nounwind {
; X64-RETPOLINE-NEXT: cmoveq %rax, %rcx
; X64-RETPOLINE-NEXT: cmpq $4, %rdx
; X64-RETPOLINE-NEXT: jne .LBB6_3
-; X64-RETPOLINE-NEXT: .Ltmp0: # Block address taken
; X64-RETPOLINE-NEXT: # %bb.6: # %bb3
; X64-RETPOLINE-NEXT: cmovneq %rax, %rcx
; X64-RETPOLINE-NEXT: shlq $47, %rcx
; X64-RETPOLINE-NEXT: movl $42, %eax
; X64-RETPOLINE-NEXT: orq %rcx, %rsp
; X64-RETPOLINE-NEXT: retq
-; X64-RETPOLINE-NEXT: .Ltmp1: # Block address taken
; X64-RETPOLINE-NEXT: .LBB6_5: # %bb2
; X64-RETPOLINE-NEXT: cmovneq %rax, %rcx
; X64-RETPOLINE-NEXT: shlq $47, %rcx
; X64-RETPOLINE-NEXT: movl $13, %eax
; X64-RETPOLINE-NEXT: orq %rcx, %rsp
; X64-RETPOLINE-NEXT: retq
-; X64-RETPOLINE-NEXT: .Ltmp2: # Block address taken
; X64-RETPOLINE-NEXT: .LBB6_4: # %bb1
; X64-RETPOLINE-NEXT: cmovneq %rax, %rcx
; X64-RETPOLINE-NEXT: shlq $47, %rcx
; X64-RETPOLINE-NEXT: movl $7, %eax
; X64-RETPOLINE-NEXT: orq %rcx, %rsp
; X64-RETPOLINE-NEXT: retq
-; X64-RETPOLINE-NEXT: .Ltmp3: # Block address taken
; X64-RETPOLINE-NEXT: .LBB6_3: # %bb0
; X64-RETPOLINE-NEXT: cmoveq %rax, %rcx
; X64-RETPOLINE-NEXT: shlq $47, %rcx
; X64-RETPOLINE-NEXT: movl $2, %eax
; X64-RETPOLINE-NEXT: orq %rcx, %rsp
; X64-RETPOLINE-NEXT: retq
+; X64-RETPOLINE-NEXT: .Ltmp0: # Address of block that was removed by CodeGen
+; X64-RETPOLINE-NEXT: .Ltmp1: # Address of block that was removed by CodeGen
+; X64-RETPOLINE-NEXT: .Ltmp2: # Address of block that was removed by CodeGen
+; X64-RETPOLINE-NEXT: .Ltmp3: # Address of block that was removed by CodeGen
entry:
%ptr = getelementptr [4 x ptr], ptr @global_blockaddrs, i32 0, i32 %idx
%a = load ptr, ptr %ptr
|
|
@llvm/pr-subscribers-llvm-globalisel Author: Justin Stitt (JustinStitt) ChangesAfter commit 4109bac ("[IR] Do not store Function inside BlockAddress"), the This was causing some objtool warnings about unreachable instructions 1. Check that BlockAddress actually has users before marking the MBB. If the BlockAddress exists but has no users (probably because they were optimized away), we can skip marking the block. The performance impact should be negligible as we still only check for users after verifying that the block address has its address taken somewhere, which should be false pretty often (I think?!). I regenerated
Nearly all of the triage was done by @nathanchance and @ernsteiswuerfel over at #167774 and ClangBuiltLinux/linux#2130. I used a kernel source that @nathanchance reduced in order to test these dead/orphaned block address placements. Full diff: https://github.com/llvm/llvm-project/pull/174480.diff 3 Files Affected:
diff --git a/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp b/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
index 12552bce3caaa..18a6dd2c15365 100644
--- a/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
@@ -4217,8 +4217,14 @@ bool IRTranslator::runOnMachineFunction(MachineFunction &CurMF) {
MBB = MF->CreateMachineBasicBlock(&BB);
MF->push_back(MBB);
- if (BB.hasAddressTaken())
- MBB->setAddressTakenIRBlock(const_cast<BasicBlock *>(&BB));
+ // Only mark the block if the BlockAddress actually has users. The
+ // hasAddressTaken flag may be stale if the BlockAddress was optimized away
+ // but the constant still exists in the uniquing table.
+ if (BB.hasAddressTaken()) {
+ if (BlockAddress *BA = BlockAddress::lookup(&BB))
+ if (!BA->use_empty())
+ MBB->setAddressTakenIRBlock(const_cast<BasicBlock *>(&BB));
+ }
if (!HasMustTailInVarArgFn)
HasMustTailInVarArgFn = checkForMustTailInVarArgFn(IsVarArg, BB);
diff --git a/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp b/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
index e73743ecbc9fa..74100eeeefcdd 100644
--- a/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
@@ -26,6 +26,7 @@
#include "llvm/CodeGen/TargetSubtargetInfo.h"
#include "llvm/CodeGen/WasmEHFuncInfo.h"
#include "llvm/CodeGen/WinEHFuncInfo.h"
+#include "llvm/IR/Constants.h"
#include "llvm/IR/DataLayout.h"
#include "llvm/IR/DerivedTypes.h"
#include "llvm/IR/Function.h"
@@ -277,8 +278,14 @@ void FunctionLoweringInfo::set(const Function &fn, MachineFunction &mf,
// Transfer the address-taken flag. This is necessary because there could
// be multiple MachineBasicBlocks corresponding to one BasicBlock, and only
// the first one should be marked.
- if (BB.hasAddressTaken())
- MBB->setAddressTakenIRBlock(const_cast<BasicBlock *>(&BB));
+ // Only mark the block if the BlockAddress actually has users. The
+ // hasAddressTaken flag may be stale if the BlockAddress was optimized away
+ // but the constant still exists in the uniquing table.
+ if (BB.hasAddressTaken()) {
+ if (BlockAddress *BA = BlockAddress::lookup(&BB))
+ if (!BA->use_empty())
+ MBB->setAddressTakenIRBlock(const_cast<BasicBlock *>(&BB));
+ }
// Mark landing pad blocks.
if (BB.isEHPad())
diff --git a/llvm/test/CodeGen/X86/speculative-load-hardening-indirect.ll b/llvm/test/CodeGen/X86/speculative-load-hardening-indirect.ll
index fd5085c8c2ac9..2092fc9cd6e00 100644
--- a/llvm/test/CodeGen/X86/speculative-load-hardening-indirect.ll
+++ b/llvm/test/CodeGen/X86/speculative-load-hardening-indirect.ll
@@ -464,34 +464,34 @@ define dso_local i32 @test_indirectbr_global(i32 %idx) nounwind {
; X64-RETPOLINE-NEXT: cmoveq %rax, %rcx
; X64-RETPOLINE-NEXT: cmpq $4, %rdx
; X64-RETPOLINE-NEXT: jne .LBB6_3
-; X64-RETPOLINE-NEXT: .Ltmp0: # Block address taken
; X64-RETPOLINE-NEXT: # %bb.6: # %bb3
; X64-RETPOLINE-NEXT: cmovneq %rax, %rcx
; X64-RETPOLINE-NEXT: shlq $47, %rcx
; X64-RETPOLINE-NEXT: movl $42, %eax
; X64-RETPOLINE-NEXT: orq %rcx, %rsp
; X64-RETPOLINE-NEXT: retq
-; X64-RETPOLINE-NEXT: .Ltmp1: # Block address taken
; X64-RETPOLINE-NEXT: .LBB6_5: # %bb2
; X64-RETPOLINE-NEXT: cmovneq %rax, %rcx
; X64-RETPOLINE-NEXT: shlq $47, %rcx
; X64-RETPOLINE-NEXT: movl $13, %eax
; X64-RETPOLINE-NEXT: orq %rcx, %rsp
; X64-RETPOLINE-NEXT: retq
-; X64-RETPOLINE-NEXT: .Ltmp2: # Block address taken
; X64-RETPOLINE-NEXT: .LBB6_4: # %bb1
; X64-RETPOLINE-NEXT: cmovneq %rax, %rcx
; X64-RETPOLINE-NEXT: shlq $47, %rcx
; X64-RETPOLINE-NEXT: movl $7, %eax
; X64-RETPOLINE-NEXT: orq %rcx, %rsp
; X64-RETPOLINE-NEXT: retq
-; X64-RETPOLINE-NEXT: .Ltmp3: # Block address taken
; X64-RETPOLINE-NEXT: .LBB6_3: # %bb0
; X64-RETPOLINE-NEXT: cmoveq %rax, %rcx
; X64-RETPOLINE-NEXT: shlq $47, %rcx
; X64-RETPOLINE-NEXT: movl $2, %eax
; X64-RETPOLINE-NEXT: orq %rcx, %rsp
; X64-RETPOLINE-NEXT: retq
+; X64-RETPOLINE-NEXT: .Ltmp0: # Address of block that was removed by CodeGen
+; X64-RETPOLINE-NEXT: .Ltmp1: # Address of block that was removed by CodeGen
+; X64-RETPOLINE-NEXT: .Ltmp2: # Address of block that was removed by CodeGen
+; X64-RETPOLINE-NEXT: .Ltmp3: # Address of block that was removed by CodeGen
entry:
%ptr = getelementptr [4 x ptr], ptr @global_blockaddrs, i32 0, i32 %idx
%a = load ptr, ptr %ptr
|
arsenm
left a comment
arsenm
left a comment
There was a problem hiding this comment.
Can you add a new test specifically for this instead of relying on the off chance test diff? The globalisel path isn't tested
| if (BB.hasAddressTaken()) { | ||
| if (BlockAddress *BA = BlockAddress::lookup(&BB)) | ||
| if (!BA->use_empty()) | ||
| MBB->setAddressTakenIRBlock(const_cast<BasicBlock *>(&BB)); |
There was a problem hiding this comment.
Added tests with 6665c48. Added x86 and arm tests where the x86 tests use retpoline to remove the indirectbr's. It should be noted that the arm test I added passes even without this PR but I included it for a bit more completeness.
nikic
left a comment
nikic
left a comment
There was a problem hiding this comment.
This looks conceptually reasonable, but I'm a bit unclear on how 4109bac caused this. Dangling blackaddress users should have existed before that change as well.
My best guess is that a removeDeadConstantUsers() call somewhere (like during Inlining or GlobalOpt) ended up removing those users as a side effect, and that no longer happens because it's no longer in the function use list, and we never call that on the BB. Does that sound plausible?
To also ignore dead users in other dead constants you can use hasZeroLiveUses() instead of use_empty(). Probably doesn't make a big difference in this case.
FWIW, this is indeed the first instance where I can observe the issue. With
I lack expertise in this area but your summary matches my initial exploration :)
SGTM, I swapped over to |
|
The current state of this pull request does resolve the warning with the full distribution configuration and original Linux source file. |
🪟 Windows x64 Test Results
✅ The build succeeded and all tests passed. |
|
I can't reproduce the test failure the bot is mentioning. I don't think that failure is my fault, can anyone confirm? |
Maybe rebase on main and re-run the CI? |
After commit 4109bac ("[IR] Do not store Function inside BlockAddress"), the `hasAddressTaken` flag on BasicBlock can become stale. When a BlockAddress constant is created, the flag is set to true. However, if optimizations remove all users of that BlockAddress (e.g., simplifying an `indirectbr` to a direct branch), the BlockAddress may remain in the uniquing table with no users, but the flag stays true. Check that BlockAddress actually has users before marking the MBB. If the BlockAddress exists but has no users (probably because they were optimized away), we can skip marking the block. The performance impact should be negligible as we still only check for users after verifying that the block address has its address taken somewhere, which should be false pretty often (I think?!). Signed-off-by: Justin Stitt <[email protected]>
Signed-off-by: Justin Stitt <[email protected]>
Signed-off-by: Justin Stitt <[email protected]>
JustinStitt
force-pushed
the
fix-llvm-167774
branch
from
7da3f90 to
dd814bc
Compare
6 participants
After commit 4109bac ("[IR] Do not store Function inside BlockAddress"), the
hasAddressTakenflag on BasicBlock can become stale. When a BlockAddress constant is created, the flag is set to true. However, if optimizations remove all users of that BlockAddress (e.g., simplifying anindirectbrto a direct branch), the BlockAddress may remain in the uniquing table with no users, but the flag stays true.This was causing some objtool warnings about unreachable instructions 1.
Check that BlockAddress actually has users before marking the MBB. If the BlockAddress exists but has no users (probably because they were optimized away), we can skip marking the block.
The performance impact should be negligible as we still only check for users after verifying that the block address has its address taken somewhere, which should be false pretty often (I think?!).
I regenerated
llvm/test/CodeGen/X86/speculative-load-hardening-indirect.llwith$ python3 llvm/utils/update_llc_test_checks.py --llc-binary build/bin/llc llvm/test/CodeGen/X86/speculative-load-hardening-indirect.llNearly all of the triage was done by @nathanchance and @ernsteiswuerfel over at #167774 and ClangBuiltLinux/linux#2130.
I used a kernel source that @nathanchance reduced in order to test these dead/orphaned block address placements.