Re: clang asm goto issue small reproducer

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

----- On Dec 15, 2021, at 7:18 PM, ndesaulniers ndesaulniers@google.com wrote:

> On Wed, Dec 15, 2021 at 3:50 PM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
>>
>> On Wed, Dec 15, 2021 at 2:06 PM Mathieu Desnoyers
>> <mathieu.desnoyers@efficios.com> wrote:
>> >
>> > Hi Nick,
>> >
>> > I have a small reproducer for the asm goto issue we discussed over irc. I have
>> > axed away all code that is not the problematic function by hand. Hopefully this
>> > won't change the overall optimization context too much.
>>
>> If I cut the compilation pipeline off at after "middle-end"
>> optimizations, I can see that `this_cpu_list_pop` returns `NULL` for
>> test.c while in test-asm.c, it's a phi node of either NULL or head
>> (which looks correct; for the test-asm.c case).
>>
>> "GVNPass" seems to be converting
>>
>> return:                                           ; preds =
>> %cleanup.thread19, %cleanup.thread
>>   %retval.118 = phi %struct.percpu_list_node* [ %9, %cleanup.thread ],
>> [ null, %cleanup.thread19 ]
>>   ret %struct.percpu_list_node* %retval.118
>>
>> into:
>>
>> return:                                           ; preds =
>> %cleanup.thread19, %cleanup.thread
>>   ret %struct.percpu_list_node* null
>>
>> Which drops the
>>   %9 = load %struct.percpu_list_node*, %struct.percpu_list_node**
>> %head, align 8, !tbaa !9
> 
> Ah, Eli pointed out that I'm chasing the wrong squirrel. From test.c we have:
> 
> struct percpu_list_node *head;
> load = (intptr_t *)&head;
> ret = rseq_cmpnev_storeoffp_load(targetptr, expectnot,
>     offset, load, cpu);
> 
> then in rseq_cmpnev_storeoffp_load():
> __asm__ __volatile__ goto (
>  :
>  :
> [load] "m" (*load));
> 
> Did we ever initialize head?  Isn't this equivalent to:
> 
> int* foo (void) {
>  int *head;
>  int **load;
>  load = &head;
>  asm(""::"m"(*load):"memory");
>  return head;
> }
> 
> Isn't that equivalent to:
> 
> int* foo (void) {
>  int *head;
>  asm(""::"m"(head):"memory");
>  return head;
> }
> 
> For which neither compiler warns for -Wuninitialized? They would if
> head was an argument to a function call...

This is a situation where I had to work around the fact that older
gcc did not support output operands. I expected that having both
a "m" input operand and a "memory" clobber would allow the asm
to write to the memory location pointed to by the "m" input operand,
and the "memory" clobber was expected to prevent the compiler
from presuming anything about memory state across the asm.

But I suspect I was expecting too much here. What would be the
best course of action to fix these asm statements ? Do I have
any way to do this without requiring a dependency on gcc/clang
that support asm goto with output operands ?

Thanks,

Mathieu


> 
>>
>> I think GVN may be eliminating a phi that it should not be.
>> Specifically, it seems that GVN thinks the default/fallthrough
>> successor is unreachable, which seems incorrect to me. The predecessor
>> lists all look correct to me, so GVN may be stumbling on callbr
>> itself.
>>
>> $ clang -O2 test.c -emit-llvm -S -o - -Xclang -disable-llvm-passes > test.ll
>> $ opt -O2 -print-before=gvn -print-after=gvn
>> -filter-print-funcs=this_cpu_list_pop test.ll -o /dev/null 2>
>> test.gvn.ll
>>
>> Then using this reproducer, I was able to narrow it down a little further:
>>
>> $ opt -O2 -print-before=gvn -print-module-scope
>> --filter-print-funcs=this_cpu_list_pop test.ll -S 2>test.pre-gvn.ll
>> $ llvm-extract --func=this_cpu_list_pop test.pre-gvn.ll -o
>> test.pre-gvn.this_cpu_list_pop.bc --recursive
>> $ llvm-dis test.pre-gvn.this_cpu_list_pop.bc
>> $ cat repro.sh
>> #!/usr/bin/env sh
>> set -e
>> grep -q "%retval.118 = phi %struct.percpu_list_node\* \[ %9,
>> %cleanup.thread \], \[ null, %cleanup.thread19 \]" $1
>> grep -q "ret %struct.percpu_list_node\* %retval.118" $1
>> OUT=$(opt -passes=gvn $1 -S -o -)
>> echo $OUT | grep -q "ret %struct.percpu_list_node\* null"
>> $ llvm-reduce --test=./repro.sh test.pre-gvn.this_cpu_list_pop.ll
>>
>> Then pared down further manually, see reduced.ll
>>
>> I've filed https://github.com/llvm/llvm-project/issues/52735, let's
>> discuss more there?
>>
>> >
>> > on x86-64:
>> >
>> > Buggy code:
>> > clang-13 -O2 -o test.o test.c
>> >
>> > OK code (added a dummy asm("")):
>> > clang-13 -O2 -o test-asm.o test-asm.c
>> >
>> > Thanks,
>> >
>> > Mathieu
>> >
>> > --
>> > Mathieu Desnoyers
>> > EfficiOS Inc.
>> > http://www.efficios.com
>>
>>
>>
>> --
>> Thanks,
>> ~Nick Desaulniers
> 
> 
> 
> --
> Thanks,
> ~Nick Desaulniers

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com