[llvm] r192636 - [X86][FastISel] During X86 fastisel, the address of indirect call was resolved

Eric Christopher echristo at gmail.com
Tue Oct 22 14:37:04 PDT 2013


Nah, thank you :)

-eric

On Tue, Oct 22, 2013 at 2:33 PM, Quentin Colombet <qcolombet at apple.com> wrote:
> Thanks committed in r193199.
>
> -Quentin
>
> On Oct 22, 2013, at 2:07 PM, Eric Christopher <echristo at gmail.com> wrote:
>
> That is awesomely verbose. It's a great comment. If you feel it helps
> the code then it'd be great to put it in :) I like more rather than
> fewer comments.
>
> -eric
>
> On Tue, Oct 22, 2013 at 2:02 PM, Quentin Colombet <qcolombet at apple.com>
> wrote:
>
> 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
>
>
>
>



More information about the llvm-commits mailing list