Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GVN-Hoist hoists a load over a predicate #32168

Closed
chandlerc opened this issue Apr 26, 2017 · 5 comments
Closed

GVN-Hoist hoists a load over a predicate #32168

chandlerc opened this issue Apr 26, 2017 · 5 comments
Labels
bugzilla Issues migrated from bugzilla

Comments

@chandlerc
Copy link
Member

Bugzilla Link 32821
Resolution FIXED
Resolved on Nov 07, 2017 15:51
Version unspecified
OS All
CC @efriedma-quic,@hiraditya,@sanjoy

Extended Description

Consider the following test case:

% cat bug.cc
class base
{
public:
base() {}
virtual ~base() {}
virtual void destroy() = 0;
virtual void destroy_deallocate() = 0;
};

class bar
{
public:
bar() : _f(0) {}
~bar()
{
if ((void *)_f == &_buf)
_f->destroy();
else if (_f)
_f->destroy_deallocate();
}

void* _buf;
base* _f;
};

extern "C" {
unsigned sleep (unsigned int __seconds);
}

extern void foo(bar x);

void baz()
{
bar x;
foo(x);
while (1) sleep(10);
}

% clang --target=x86_64-unknown-unknown -S -o - bug.cc -O2 -fno-exceptions -fno-rtti -mllvm -enable-gvn-hoist=false
.text
.file "bug.cc"
.globl _Z3bazv
.p2align 4, 0x90
.type _Z3bazv,@function
_Z3bazv: # @​_Z3bazv
.cfi_startproc

BB#0: # %entry

    pushq   %rbp

.Lcfi0:
.cfi_def_cfa_offset 16
.Lcfi1:
.cfi_offset %rbp, -16
movq %rsp, %rbp
.Lcfi2:
.cfi_def_cfa_register %rbp
pushq %rbx
subq $24, %rsp
.Lcfi3:
.cfi_offset %rbx, -24
movq $0, -16(%rbp)
leaq -24(%rbp), %rbx
movq %rbx, %rdi
callq _Z3foo3bar
movq -16(%rbp), %rdi
cmpq %rbx, %rdi
je .LBB0_1

BB#2: # %if.else.i

    testq   %rdi, %rdi              # NULL TEST
    je      .LBB0_4

BB#3: # %if.then4.i

    movq    (%rdi), %rax            # LOAD
    callq   *24(%rax)
    jmp     .LBB0_4

.LBB0_1: # %if.then.i
movq (%rdi), %rax
callq *16(%rax)
.p2align 4, 0x90
.LBB0_4: # %while.cond
# =>This Inner Loop Header: Depth=1
movl $10, %edi
callq sleep
jmp .LBB0_4
.Lfunc_end0:
.size _Z3bazv, .Lfunc_end0-_Z3bazv
.cfi_endproc

    .ident  "clang version 5.0.0 (trunk 301138) (llvm/trunk 301148)"
    .section        ".note.GNU-stack","",@progbits

versus:

% clang --target=x86_64-unknown-unknown -S -o - bug.cc -O2 -fno-exceptions -fno-rtti -mllvm -enable-gvn-hoist=true
.text
.file "bug.cc"
.globl _Z3bazv
.p2align 4, 0x90
.type _Z3bazv,@function
_Z3bazv: # @​_Z3bazv
.cfi_startproc

BB#0: # %entry

    pushq   %rbp

.Lcfi0:
.cfi_def_cfa_offset 16
.Lcfi1:
.cfi_offset %rbp, -16
movq %rsp, %rbp
.Lcfi2:
.cfi_def_cfa_register %rbp
pushq %rbx
subq $24, %rsp
.Lcfi3:
.cfi_offset %rbx, -24
movq $0, -16(%rbp)
leaq -24(%rbp), %rbx
movq %rbx, %rdi
callq _Z3foo3bar
movq -16(%rbp), %rdi
cmpq %rbx, %rdi
movq (%rdi), %rax # CRASH
je .LBB0_1

BB#2: # %if.else.i

    testq   %rdi, %rdi              # NULL TEST
    je      .LBB0_4

BB#3: # %if.then4.i

    callq   *24(%rax)
    jmp     .LBB0_4

.LBB0_1: # %if.then.i
callq *16(%rax)
.p2align 4, 0x90
.LBB0_4: # %while.cond
# =>This Inner Loop Header: Depth=1
movl $10, %edi
callq sleep
jmp .LBB0_4
.Lfunc_end0:
.size _Z3bazv, .Lfunc_end0-_Z3bazv
.cfi_endproc

    .ident  "clang version 5.0.0 (trunk 301138) (llvm/trunk 301148)"
    .section        ".note.GNU-stack","",@progbits

See the lines marked "NULL TEST", "LOAD", and "CRASH". GVN Hoist is lifting the load through %rdi in this example across the null test.

You can also see this in the IR, nothing x86 specific here.

@chandlerc
Copy link
Member Author

Disabled in r301505 while this is investigated.

@hiraditya
Copy link
Collaborator

Thanks for the test case. I can confirm that the bug is in gvn-hoist. It is unable to determine the availability on all paths because of an infinite loop (because of no path to the end of the function). I'm working on the fix.

@hiraditya
Copy link
Collaborator

Patch for review: https://reviews.llvm.org/D32614

@efriedma-quic
Copy link
Collaborator

Did this get fixed?

@hiraditya
Copy link
Collaborator

Yes, the latest gvnhoist has the fixes. Fixed in: https://reviews.llvm.org/D35918

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla
Projects
None yet
Development

No branches or pull requests

3 participants