[llvm] r192636 - [X86][FastISel] During X86 fastisel, the address of indirect call was resolved
Quentin Colombet
qcolombet at apple.com
Tue Oct 22 14:02:51 PDT 2013
Hi Eric,
I tried to comment that part of the code, but it is quite verbose.
Anyway, what do you think of the attached patch?
Thanks,
-Quentin
On Oct 22, 2013, at 1:34 PM, Eric Christopher <echristo at gmail.com> wrote:
> I seem to want the check-in comment somewhere in the code highlighting
> the problem that would occur otherwise, but I guess having a failing
> test is good enough. Anyhow, just a "if you can come up with some way
> to elaborate in the code then awesome" comment :)
>
> -eric
>
> On Mon, Oct 14, 2013 at 3:32 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>> Author: qcolombet
>> Date: Mon Oct 14 17:32:09 2013
>> New Revision: 192636
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=192636&view=rev
>> Log:
>> [X86][FastISel] During X86 fastisel, the address of indirect call was resolved
>> through bitcast, ptrtoint, and inttoptr instructions. This is valid
>> only if the related instructions are in that same basic block, otherwise
>> we may reference variables that were not live accross basic blocks
>> resulting in undefined virtual registers.
>>
>> The bug was exposed when both SDISel and FastISel were used within the same
>> function, i.e., one basic block is issued with FastISel and another with SDISel,
>> as demonstrated with the testcase.
>>
>> <rdar://problem/15192473>
>>
>> Added:
>> llvm/trunk/test/CodeGen/X86/2013-10-14-FastISel-incorrect-vreg.ll
>> Modified:
>> llvm/trunk/lib/Target/X86/X86FastISel.cpp
>>
>> Modified: llvm/trunk/lib/Target/X86/X86FastISel.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86FastISel.cpp?rev=192636&r1=192635&r2=192636&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Target/X86/X86FastISel.cpp (original)
>> +++ llvm/trunk/lib/Target/X86/X86FastISel.cpp Mon Oct 14 17:32:09 2013
>> @@ -632,9 +632,12 @@ redo_gep:
>> bool X86FastISel::X86SelectCallAddress(const Value *V, X86AddressMode &AM) {
>> const User *U = NULL;
>> unsigned Opcode = Instruction::UserOp1;
>> - if (const Instruction *I = dyn_cast<Instruction>(V)) {
>> + const Instruction *I = dyn_cast<Instruction>(V);
>> + bool InMBB = true;
>> + if (I) {
>> Opcode = I->getOpcode();
>> U = I;
>> + InMBB = I->getParent() == FuncInfo.MBB->getBasicBlock();
>> } else if (const ConstantExpr *C = dyn_cast<ConstantExpr>(V)) {
>> Opcode = C->getOpcode();
>> U = C;
>> @@ -643,18 +646,22 @@ bool X86FastISel::X86SelectCallAddress(c
>> switch (Opcode) {
>> default: break;
>> case Instruction::BitCast:
>> - // Look past bitcasts.
>> - return X86SelectCallAddress(U->getOperand(0), AM);
>> + // Look past bitcasts if its operand is in the same BB.
>> + if (InMBB)
>> + return X86SelectCallAddress(U->getOperand(0), AM);
>> + break;
>>
>> case Instruction::IntToPtr:
>> - // Look past no-op inttoptrs.
>> - if (TLI.getValueType(U->getOperand(0)->getType()) == TLI.getPointerTy())
>> + // Look past no-op inttoptrs if its operand is in the same BB.
>> + if (InMBB &&
>> + TLI.getValueType(U->getOperand(0)->getType()) == TLI.getPointerTy())
>> return X86SelectCallAddress(U->getOperand(0), AM);
>> break;
>>
>> case Instruction::PtrToInt:
>> - // Look past no-op ptrtoints.
>> - if (TLI.getValueType(U->getType()) == TLI.getPointerTy())
>> + // Look past no-op ptrtoints if its operand is in the same BB.
>> + if (InMBB &&
>> + TLI.getValueType(U->getType()) == TLI.getPointerTy())
>> return X86SelectCallAddress(U->getOperand(0), AM);
>> break;
>> }
>>
>> Added: llvm/trunk/test/CodeGen/X86/2013-10-14-FastISel-incorrect-vreg.ll
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/2013-10-14-FastISel-incorrect-vreg.ll?rev=192636&view=auto
>> ==============================================================================
>> --- llvm/trunk/test/CodeGen/X86/2013-10-14-FastISel-incorrect-vreg.ll (added)
>> +++ llvm/trunk/test/CodeGen/X86/2013-10-14-FastISel-incorrect-vreg.ll Mon Oct 14 17:32:09 2013
>> @@ -0,0 +1,132 @@
>> +; RUN: llc -mtriple x86_64-apple-darwin -O0 < %s -o - | FileCheck %s
>> +;
>> +; During X86 fastisel, the address of indirect call was resolved
>> +; through bitcast, ptrtoint, and inttoptr instructions. This is valid
>> +; only if the related instructions are in that same basic block, otherwise
>> +; we may reference variables that were not live accross basic blocks
>> +; resulting in undefined virtual registers.
>> +;
>> +; In this example, this is illustrated by a the spill/reload of the
>> +; LOADED_PTR_SLOT.
>> +;
>> +; Before this patch, the compiler was accessing two different spill
>> +; slots.
>> +; <rdar://problem/15192473>
>> +
>> +; CHECK-LABEL: @test_bitcast
>> +; Load the value of the function pointer: %loaded_ptr
>> +; CHECK: movq (%rdi), [[LOADED_PTR:%[a-z]+]]
>> +; Spill %arg2.
>> +; CHECK: movq %rdx, [[ARG2_SLOT:[0-9]*\(%[a-z]+\)]]
>> +; Spill %loaded_ptr.
>> +; CHECK: movq [[LOADED_PTR]], [[LOADED_PTR_SLOT:[0-9]*\(%[a-z]+\)]]
>> +; Perform the indirect call.
>> +; Load the first argument
>> +; CHECK: movq [[ARG2_SLOT]], %rdi
>> +; Load the second argument
>> +; CHECK: movq [[ARG2_SLOT]], %rsi
>> +; Load the thrid argument
>> +; CHECK: movq [[ARG2_SLOT]], %rdx
>> +; Load the function pointer.
>> +; CHECK: movq [[LOADED_PTR_SLOT]], [[FCT_PTR:%[a-z]+]]
>> +; Call.
>> +; CHECK: callq *[[FCT_PTR]]
>> +; CHECK: ret
>> +define i64 @test_bitcast(i64 (i64, i64, i64)** %arg, i1 %bool, i64 %arg2) {
>> +entry:
>> + %loaded_ptr = load i64 (i64, i64, i64)** %arg, align 8
>> + %raw = bitcast i64 (i64, i64, i64)* %loaded_ptr to i8*
>> + switch i1 %bool, label %default [
>> + i1 true, label %label_true
>> + i1 false, label %label_end
>> + ]
>> +default:
>> + unreachable
>> +
>> +label_true:
>> + br label %label_end
>> +
>> +label_end:
>> + %fct_ptr = bitcast i8* %raw to i64 (i64, i64, i64)*
>> + %res = call i64 %fct_ptr(i64 %arg2, i64 %arg2, i64 %arg2)
>> + ret i64 %res
>> +}
>> +
>> +; CHECK-LABEL: @test_inttoptr
>> +; Load the value of the function pointer: %loaded_ptr
>> +; CHECK: movq (%rdi), [[LOADED_PTR:%[a-z]+]]
>> +; Spill %arg2.
>> +; CHECK: movq %rdx, [[ARG2_SLOT:[0-9]*\(%[a-z]+\)]]
>> +; Spill %loaded_ptr.
>> +; CHECK: movq [[LOADED_PTR]], [[LOADED_PTR_SLOT:[0-9]*\(%[a-z]+\)]]
>> +; Perform the indirect call.
>> +; Load the first argument
>> +; CHECK: movq [[ARG2_SLOT]], %rdi
>> +; Load the second argument
>> +; CHECK: movq [[ARG2_SLOT]], %rsi
>> +; Load the thrid argument
>> +; CHECK: movq [[ARG2_SLOT]], %rdx
>> +; Load the function pointer.
>> +; CHECK: movq [[LOADED_PTR_SLOT]], [[FCT_PTR:%[a-z]+]]
>> +; Call.
>> +; CHECK: callq *[[FCT_PTR]]
>> +; CHECK: ret
>> +define i64 @test_inttoptr(i64 (i64, i64, i64)** %arg, i1 %bool, i64 %arg2) {
>> +entry:
>> + %loaded_ptr = load i64 (i64, i64, i64)** %arg, align 8
>> + %raw = ptrtoint i64 (i64, i64, i64)* %loaded_ptr to i64
>> + switch i1 %bool, label %default [
>> + i1 true, label %label_true
>> + i1 false, label %label_end
>> + ]
>> +default:
>> + unreachable
>> +
>> +label_true:
>> + br label %label_end
>> +
>> +label_end:
>> + %fct_ptr = inttoptr i64 %raw to i64 (i64, i64, i64)*
>> + %res = call i64 %fct_ptr(i64 %arg2, i64 %arg2, i64 %arg2)
>> + ret i64 %res
>> +}
>> +
>> +; CHECK-LABEL: @test_ptrtoint
>> +; Load the value of the function pointer: %loaded_ptr
>> +; CHECK: movq (%rdi), [[LOADED_PTR:%[a-z]+]]
>> +; Spill %arg2.
>> +; CHECK: movq %rdx, [[ARG2_SLOT:[0-9]*\(%[a-z]+\)]]
>> +; Spill %loaded_ptr.
>> +; CHECK: movq [[LOADED_PTR]], [[LOADED_PTR_SLOT:[0-9]*\(%[a-z]+\)]]
>> +; Perform the indirect call.
>> +; Load the first argument
>> +; CHECK: movq [[ARG2_SLOT]], %rdi
>> +; Load the second argument
>> +; CHECK: movq [[ARG2_SLOT]], %rsi
>> +; Load the thrid argument
>> +; CHECK: movq [[ARG2_SLOT]], %rdx
>> +; Load the function pointer.
>> +; CHECK: movq [[LOADED_PTR_SLOT]], [[FCT_PTR:%[a-z]+]]
>> +; Call.
>> +; CHECK: callq *[[FCT_PTR]]
>> +; CHECK: ret
>> +define i64 @test_ptrtoint(i64 (i64, i64, i64)** %arg, i1 %bool, i64 %arg2) {
>> +entry:
>> + %loaded_ptr = load i64 (i64, i64, i64)** %arg, align 8
>> + %raw = bitcast i64 (i64, i64, i64)* %loaded_ptr to i8*
>> + switch i1 %bool, label %default [
>> + i1 true, label %label_true
>> + i1 false, label %label_end
>> + ]
>> +default:
>> + unreachable
>> +
>> +label_true:
>> + br label %label_end
>> +
>> +label_end:
>> + %fct_int = ptrtoint i8* %raw to i64
>> + %fct_ptr = inttoptr i64 %fct_int to i64 (i64, i64, i64)*
>> + %res = call i64 %fct_ptr(i64 %arg2, i64 %arg2, i64 %arg2)
>> + ret i64 %res
>> +}
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131022/c38ac5cb/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: X86FastISel.patch
Type: application/octet-stream
Size: 1806 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131022/c38ac5cb/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131022/c38ac5cb/attachment-0001.html>
More information about the llvm-commits
mailing list