From 4e1d389ac5cf5048d2c4e90caa2de672d7445148 Mon Sep 17 00:00:00 2001 From: Quentin Colombet Date: Mon, 2 May 2016 22:58:54 +0000 Subject: [PATCH] [X86] Model FAULTING_LOAD_OP as a terminator and branch. This operation may branch to the handler block and we do not want it to happen anywhere within the basic block. Moreover, by marking it "terminator and branch" the machine verifier does not wrongly assume (because of AnalyzeBranch not knowing better) the branch is analyzable. Indeed, the target was seeing only the unconditional branch and not the faulting load op and thought it was a simple unconditional block. The machine verifier was complaining because of that and moreover, other optimizations could have done wrong transformation! In the process, simplify the representation of the handler block in the faulting load op. Now, we directly reference the handler block instead of using a label. This has the benefits of: 1. MC knows how to issue a label for a BB, so leave that to it. 2. Accessing the target BB from its label is painful, whereas it is direct from a MBB operand. Note: The 2 bytes offset in implicit-null-check.ll comes from the fact the unconditional jumps are not removed anymore, as the whole terminator sequence is not analyzable anymore. Will fix it in a subsequence commit. llvm-svn: 268327 --- llvm/include/llvm/Target/Target.td | 2 + llvm/lib/CodeGen/ImplicitNullChecks.cpp | 21 +++----- llvm/lib/Target/X86/X86MCInstLower.cpp | 4 +- llvm/test/CodeGen/X86/implicit-null-check.ll | 52 ++++++++++---------- 4 files changed, 38 insertions(+), 41 deletions(-) diff --git a/llvm/include/llvm/Target/Target.td b/llvm/include/llvm/Target/Target.td index f7fe904f61f3..4c6833ed530c 100644 --- a/llvm/include/llvm/Target/Target.td +++ b/llvm/include/llvm/Target/Target.td @@ -928,6 +928,8 @@ def FAULTING_LOAD_OP : Instruction { let InOperandList = (ins variable_ops); let usesCustomInserter = 1; let mayLoad = 1; + let isTerminator = 1; + let isBranch = 1; } def PATCHABLE_OP : Instruction { let OutOperandList = (outs unknown:$dst); diff --git a/llvm/lib/CodeGen/ImplicitNullChecks.cpp b/llvm/lib/CodeGen/ImplicitNullChecks.cpp index c923ddd94715..dfd1be8c8eae 100644 --- a/llvm/lib/CodeGen/ImplicitNullChecks.cpp +++ b/llvm/lib/CodeGen/ImplicitNullChecks.cpp @@ -95,7 +95,7 @@ class ImplicitNullChecks : public MachineFunctionPass { bool analyzeBlockForNullChecks(MachineBasicBlock &MBB, SmallVectorImpl &NullCheckList); MachineInstr *insertFaultingLoad(MachineInstr *LoadMI, MachineBasicBlock *MBB, - MCSymbol *HandlerLabel); + MachineBasicBlock *HandlerMBB); void rewriteNullChecks(ArrayRef NullCheckList); public: @@ -349,11 +349,12 @@ bool ImplicitNullChecks::analyzeBlockForNullChecks( /// Wrap a machine load instruction, LoadMI, into a FAULTING_LOAD_OP machine /// instruction. The FAULTING_LOAD_OP instruction does the same load as LoadMI -/// (defining the same register), and branches to HandlerLabel if the load +/// (defining the same register), and branches to HandlerMBB if the load /// faults. The FAULTING_LOAD_OP instruction is inserted at the end of MBB. -MachineInstr *ImplicitNullChecks::insertFaultingLoad(MachineInstr *LoadMI, - MachineBasicBlock *MBB, - MCSymbol *HandlerLabel) { +MachineInstr * +ImplicitNullChecks::insertFaultingLoad(MachineInstr *LoadMI, + MachineBasicBlock *MBB, + MachineBasicBlock *HandlerMBB) { const unsigned NoRegister = 0; // Guaranteed to be the NoRegister value for // all targets. @@ -369,7 +370,7 @@ MachineInstr *ImplicitNullChecks::insertFaultingLoad(MachineInstr *LoadMI, } auto MIB = BuildMI(MBB, DL, TII->get(TargetOpcode::FAULTING_LOAD_OP), DefReg) - .addSym(HandlerLabel) + .addMBB(HandlerMBB) .addImm(LoadMI->getOpcode()); for (auto &MO : LoadMI->uses()) @@ -386,8 +387,6 @@ void ImplicitNullChecks::rewriteNullChecks( DebugLoc DL; for (auto &NC : NullCheckList) { - MCSymbol *HandlerLabel = MMI->getContext().createTempSymbol(); - // Remove the conditional branch dependent on the null check. unsigned BranchesRemoved = TII->RemoveBranch(*NC.CheckBlock); (void)BranchesRemoved; @@ -398,7 +397,7 @@ void ImplicitNullChecks::rewriteNullChecks( // touch the successors list for any basic block since we haven't changed // control flow, we've just made it implicit. MachineInstr *FaultingLoad = - insertFaultingLoad(NC.MemOperation, NC.CheckBlock, HandlerLabel); + insertFaultingLoad(NC.MemOperation, NC.CheckBlock, NC.NullSucc); // Now the value of the MemOperation, if any, is live-in of block // of MemOperation. unsigned Reg = FaultingLoad->getOperand(0).getReg(); @@ -414,10 +413,6 @@ void ImplicitNullChecks::rewriteNullChecks( TII->InsertBranch(*NC.CheckBlock, NC.NotNullSucc, nullptr, /*Cond=*/None, DL); - // Emit the HandlerLabel as an EH_LABEL. - BuildMI(*NC.NullSucc, NC.NullSucc->begin(), DL, - TII->get(TargetOpcode::EH_LABEL)).addSym(HandlerLabel); - NumImplicitNullChecks++; } } diff --git a/llvm/lib/Target/X86/X86MCInstLower.cpp b/llvm/lib/Target/X86/X86MCInstLower.cpp index 4f194cac462a..83c99264dc77 100644 --- a/llvm/lib/Target/X86/X86MCInstLower.cpp +++ b/llvm/lib/Target/X86/X86MCInstLower.cpp @@ -931,10 +931,10 @@ void X86AsmPrinter::LowerSTATEPOINT(const MachineInstr &MI, void X86AsmPrinter::LowerFAULTING_LOAD_OP(const MachineInstr &MI, X86MCInstLower &MCIL) { - // FAULTING_LOAD_OP , , , + // FAULTING_LOAD_OP , , , unsigned LoadDefRegister = MI.getOperand(0).getReg(); - MCSymbol *HandlerLabel = MI.getOperand(1).getMCSymbol(); + MCSymbol *HandlerLabel = MI.getOperand(1).getMBB()->getSymbol(); unsigned LoadOpcode = MI.getOperand(2).getImm(); unsigned LoadOperandsBeginIdx = 3; diff --git a/llvm/test/CodeGen/X86/implicit-null-check.ll b/llvm/test/CodeGen/X86/implicit-null-check.ll index 8b905f5d23b6..166408de529b 100644 --- a/llvm/test/CodeGen/X86/implicit-null-check.ll +++ b/llvm/test/CodeGen/X86/implicit-null-check.ll @@ -12,10 +12,10 @@ define i32 @imp_null_check_load(i32* %x) { ; CHECK-LABEL: _imp_null_check_load: -; CHECK: Ltmp1: +; CHECK: [[BB0_imp_null_check_load:L[^:]+]]: ; CHECK: movl (%rdi), %eax ; CHECK: retq -; CHECK: Ltmp0: +; CHECK: [[BB1_imp_null_check_load:LBB0_[0-9]+]]: ; CHECK: movl $42, %eax ; CHECK: retq @@ -33,10 +33,10 @@ define i32 @imp_null_check_load(i32* %x) { define i32 @imp_null_check_gep_load(i32* %x) { ; CHECK-LABEL: _imp_null_check_gep_load: -; CHECK: Ltmp3: +; CHECK: [[BB0_imp_null_check_gep_load:L[^:]+]]: ; CHECK: movl 128(%rdi), %eax ; CHECK: retq -; CHECK: Ltmp2: +; CHECK: [[BB1_imp_null_check_gep_load:LBB1_[0-9]+]]: ; CHECK: movl $42, %eax ; CHECK: retq @@ -55,11 +55,11 @@ define i32 @imp_null_check_gep_load(i32* %x) { define i32 @imp_null_check_add_result(i32* %x, i32 %p) { ; CHECK-LABEL: _imp_null_check_add_result: -; CHECK: Ltmp5: +; CHECK: [[BB0_imp_null_check_add_result:L[^:]+]]: ; CHECK: addl (%rdi), %esi ; CHECK: movl %esi, %eax ; CHECK: retq -; CHECK: Ltmp4: +; CHECK: [[BB1_imp_null_check_add_result:LBB2_[0-9]+]]: ; CHECK: movl $42, %eax ; CHECK: retq @@ -78,12 +78,12 @@ define i32 @imp_null_check_add_result(i32* %x, i32 %p) { define i32 @imp_null_check_hoist_over_unrelated_load(i32* %x, i32* %y, i32* %z) { ; CHECK-LABEL: _imp_null_check_hoist_over_unrelated_load: -; CHECK: Ltmp7: +; CHECK: [[BB0_imp_null_check_hoist_over_unrelated_load:L[^:]+]]: ; CHECK: movl (%rdi), %eax ; CHECK: movl (%rsi), %ecx ; CHECK: movl %ecx, (%rdx) ; CHECK: retq -; CHECK: Ltmp6: +; CHECK: [[BB1_imp_null_check_hoist_over_unrelated_load:LBB3_[0-9]+]]: ; CHECK: movl $42, %eax ; CHECK: retq @@ -103,12 +103,12 @@ define i32 @imp_null_check_hoist_over_unrelated_load(i32* %x, i32* %y, i32* %z) define i32 @imp_null_check_via_mem_comparision(i32* %x, i32 %val) { ; CHECK-LABEL: _imp_null_check_via_mem_comparision -; CHECK: Ltmp9: +; CHECK: [[BB0_imp_null_check_via_mem_comparision:L[^:]+]]: ; CHECK: cmpl %esi, 4(%rdi) ; CHECK: jge LBB4_2 ; CHECK: movl $100, %eax ; CHECK: retq -; CHECK: Ltmp8: +; CHECK: [[BB1_imp_null_check_via_mem_comparision:LBB4_[0-9]+]]: ; CHECK: movl $42, %eax ; CHECK: retq ; CHECK: LBB4_2: @@ -158,9 +158,9 @@ define i32 @imp_null_check_via_mem_comparision(i32* %x, i32 %val) { ; Fault[0].Type: ; CHECK-NEXT: .long 1 ; Fault[0].FaultOffset: -; CHECK-NEXT: .long Ltmp5-_imp_null_check_add_result +; CHECK-NEXT: .long [[BB0_imp_null_check_add_result]]-_imp_null_check_add_result ; Fault[0].HandlerOffset: -; CHECK-NEXT: .long Ltmp4-_imp_null_check_add_result +; CHECK-NEXT: .long [[BB1_imp_null_check_add_result]]-_imp_null_check_add_result ; FunctionAddr: ; CHECK-NEXT: .quad _imp_null_check_gep_load @@ -171,9 +171,9 @@ define i32 @imp_null_check_via_mem_comparision(i32* %x, i32 %val) { ; Fault[0].Type: ; CHECK-NEXT: .long 1 ; Fault[0].FaultOffset: -; CHECK-NEXT: .long Ltmp3-_imp_null_check_gep_load +; CHECK-NEXT: .long [[BB0_imp_null_check_gep_load]]-_imp_null_check_gep_load ; Fault[0].HandlerOffset: -; CHECK-NEXT: .long Ltmp2-_imp_null_check_gep_load +; CHECK-NEXT: .long [[BB1_imp_null_check_gep_load]]-_imp_null_check_gep_load ; FunctionAddr: ; CHECK-NEXT: .quad _imp_null_check_hoist_over_unrelated_load @@ -184,9 +184,9 @@ define i32 @imp_null_check_via_mem_comparision(i32* %x, i32 %val) { ; Fault[0].Type: ; CHECK-NEXT: .long 1 ; Fault[0].FaultOffset: -; CHECK-NEXT: .long Ltmp7-_imp_null_check_hoist_over_unrelated_load +; CHECK-NEXT: .long [[BB0_imp_null_check_hoist_over_unrelated_load]]-_imp_null_check_hoist_over_unrelated_load ; Fault[0].HandlerOffset: -; CHECK-NEXT: .long Ltmp6-_imp_null_check_hoist_over_unrelated_load +; CHECK-NEXT: .long [[BB1_imp_null_check_hoist_over_unrelated_load]]-_imp_null_check_hoist_over_unrelated_load ; FunctionAddr: ; CHECK-NEXT: .quad _imp_null_check_load @@ -197,9 +197,9 @@ define i32 @imp_null_check_via_mem_comparision(i32* %x, i32 %val) { ; Fault[0].Type: ; CHECK-NEXT: .long 1 ; Fault[0].FaultOffset: -; CHECK-NEXT: .long Ltmp1-_imp_null_check_load +; CHECK-NEXT: .long [[BB0_imp_null_check_load]]-_imp_null_check_load ; Fault[0].HandlerOffset: -; CHECK-NEXT: .long Ltmp0-_imp_null_check_load +; CHECK-NEXT: .long [[BB1_imp_null_check_load]]-_imp_null_check_load ; FunctionAddr: ; CHECK-NEXT: .quad _imp_null_check_via_mem_comparision @@ -210,18 +210,18 @@ define i32 @imp_null_check_via_mem_comparision(i32* %x, i32 %val) { ; Fault[0].Type: ; CHECK-NEXT: .long 1 ; Fault[0].FaultOffset: -; CHECK-NEXT: .long Ltmp9-_imp_null_check_via_mem_comparision +; CHECK-NEXT: .long [[BB0_imp_null_check_via_mem_comparision]]-_imp_null_check_via_mem_comparision ; Fault[0].HandlerOffset: -; CHECK-NEXT: .long Ltmp8-_imp_null_check_via_mem_comparision +; CHECK-NEXT: .long [[BB1_imp_null_check_via_mem_comparision]]-_imp_null_check_via_mem_comparision ; OBJDUMP: FaultMap table: ; OBJDUMP-NEXT: Version: 0x1 ; OBJDUMP-NEXT: NumFunctions: 5 ; OBJDUMP-NEXT: FunctionAddress: 0x000000, NumFaultingPCs: 1 +; OBJDUMP-NEXT: Fault kind: FaultingLoad, faulting PC offset: 0, handling PC offset: 7 +; OBJDUMP-NEXT: FunctionAddress: 0x000000, NumFaultingPCs: 1 +; OBJDUMP-NEXT: Fault kind: FaultingLoad, faulting PC offset: 0, handling PC offset: 9 +; OBJDUMP-NEXT: FunctionAddress: 0x000000, NumFaultingPCs: 1 +; OBJDUMP-NEXT: Fault kind: FaultingLoad, faulting PC offset: 0, handling PC offset: 9 +; OBJDUMP-NEXT: FunctionAddress: 0x000000, NumFaultingPCs: 1 ; OBJDUMP-NEXT: Fault kind: FaultingLoad, faulting PC offset: 0, handling PC offset: 5 -; OBJDUMP-NEXT: FunctionAddress: 0x000000, NumFaultingPCs: 1 -; OBJDUMP-NEXT: Fault kind: FaultingLoad, faulting PC offset: 0, handling PC offset: 7 -; OBJDUMP-NEXT: FunctionAddress: 0x000000, NumFaultingPCs: 1 -; OBJDUMP-NEXT: Fault kind: FaultingLoad, faulting PC offset: 0, handling PC offset: 7 -; OBJDUMP-NEXT: FunctionAddress: 0x000000, NumFaultingPCs: 1 -; OBJDUMP-NEXT: Fault kind: FaultingLoad, faulting PC offset: 0, handling PC offset: 3