[PATCH] D103458: [SLP]Improve gathering of scalar elements.

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added a comment.

In D103458#2850764 <https://reviews.llvm.org/D103458#2850764>, @Carrot wrote:

> The problem is exactly in the merging of two undef into poison.
>
> In our original code, the function foo is actually a class constructor, it initializes two bit fields
>
>   entry:
>      ...
>      br %cond, label %then, label %bug
>   
>   then:
>      call baz()
>      br label %bug
>   
>   bug:
>     word1 = load i64 bitfield1                          
>     word1.clear = and i64 word1, mask1
>     word1.set = or i64 word1.clear, value1            // set bitfield2
>     store i64 word1.set, bitfield1                    // store bitfield1
>     word2 = load i64 bitfiled2
>     word2.clear = and i64 word2, mask2
>     word2.set = or i64 word2.clear, value2           // set bitfield2
>     store i64 word2.set, bitfield2                   // store bitfield2
>     bar()
>     ret
>   
>   Notice that word1 is a 64b integer which contains bitfield1 and some other bitfields, 
>   since it is in constructor, these fields have not been initialized, so word1 is actually undef, 
>   LLVM doesn't recognize it because there is a function call in block %then (without precise 
>   alias analysis). There is similar conclusion with word2.
>   
>   In GVN pass, it has the knowledge that word1 must have undef value from control flow
>   %entry -> %bug, so the load instruction can be deleted in this path, but not sure from the
>    control flow %then -> %bug, so it must keep the load in block %then, similar analysis applies
>   to word2, so it changes the IR into the form 
>   
>   entry:
>     %nb = alloca %S, align 8
>     br i1 %cond, label %then, label %bug
>   
>   then:
>     call void @baz()
>     %ptr_2 = getelementptr inbounds %S, %S* %nb, i64 0, i32 0
>     %load2 = load i64, i64* %ptr_2, align 8
>     %bf_1 = getelementptr inbounds %S, %S* %nb, i64 0, i32 1
>     %ptr_1 = bitcast i40* %bf_1 to i64*
>     %load1 = load i64, i64* %ptr_1, align 8
>     br label %bug
>   
>   bug:
>     %word1 = phi i64 [ %load1, %then ], [ undef, %entry ]
>     %word2 = phi i64 [ %load2, %then ], [ undef, %entry ]
>     %ptr2 = getelementptr inbounds %S, %S* %nb, i64 0, i32 0
>     %word2.clear = and i64 %word2, -1152921504606846976
>     %word2.set = or i64 %word2.clear, 1
>     store i64 %word2.set, i64* %ptr2, align 8
>     %bf1 = getelementptr inbounds %S, %S* %nb, i64 0, i32 1
>     %ptr1 = bitcast i40* %bf1 to i64*
>     %word1.clear = and i64 %word1, -68719476736
>     %word1.set = or i64 %word1.clear, 21
>     store i64 %word1.set, i64* %ptr1, align 8
>     call void @bar()
>     br label %exit
>
> So the two undef values are actually correct and reasonable. But a poison value makes the code completely wrong.
> According to https://llvm.org/docs/LangRef.html#poison-values, A poison value is a result of an erroneous operation.
> There is no erroneous operation here.

Alive does not agree with your analysis. Also, it is not SLP who merges undefs into poison, `Builder.CreateInsertElement` does this magic. Also, https://llvm.org/docs/LangRef.html#poison-values says that phis do not depend on the operands, so phi is not poisoned.

To me, the code is strange. You're allocating a variable on the stack, initialize it and then just exit from the function and the variable is not used anyware. Of course optimizer optimizes it out and all operations with it. To me, looks like something wrong with the code or other transformations, if the code is correct, SLP just reveals it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103458/new/

https://reviews.llvm.org/D103458