HomePhabricator

[Constant] Update ConstantVector::get to return poison if all input elems are…

Authored by aqjune on Jan 6 2021, 4:24 PM.

Description

[Constant] Update ConstantVector::get to return poison if all input elems are poison

The diff was reviewed at D93994

Details

Committed
aqjuneJan 6 2021, 4:26 PM
Parents
rG8deaec122ec6: [analyzer] Update Fuchsia checker to catch releasing unowned handles.
Branches
Unknown
Tags
Unknown

Event Timeline

Hi,

I'm not sure what happens yet, but with this patch I'm seeing a miscompile.
Diffs start occuring after the loop vectorizer. Previously we got this store

store <4 x i32> zeroinitializer, <4 x i32>* %8, align 1, !dbg !16

but now we instead get this:

store <4 x i32> poison, <4 x i32>* %8, align 1, !dbg !16

Are you aware of any problems?

I've been able to reproduce the problem I see for an in-tree architecture.
So with

opt -mtriple=s390x-unknown-linux -mcpu=z13 -loop-vectorize -S -o - bbi-51947.ll

I get

store <4 x i32> poison, <4 x i32>* %6, align 1

but before this patch I got

store <4 x i32> zeroinitializer, <4 x i32>* %6, align 1

The input is

%arrayt = type [64 x i32]

@v_146 = external global %arrayt, align 1

define void @foo() {
entry:
  br label %for.cond

for.cond:                                         ; preds = %cond.end, %entry
  %storemerge = phi i16 [ 0, %entry ], [ %inc, %cond.end ]
  %cmp = icmp slt i16 %storemerge, 15
  br i1 %cmp, label %for.body, label %for.end

for.body:                                         ; preds = %for.cond
  br i1 true, label %cond.false, label %land.rhs

land.rhs:                                         ; preds = %for.body
  br i1 poison, label %cond.end, label %cond.false

cond.false:                                       ; preds = %for.body, %land.rhs
  br label %cond.end

cond.end:                                         ; preds = %land.rhs, %cond.false
  %cond = phi i32 [ 0, %cond.false ], [ 1, %land.rhs ]
  %arrayidx = getelementptr inbounds %arrayt, %arrayt* @v_146, i16 0, i16 %storemerge
  store i32 %cond, i32* %arrayidx, align 1
  %inc = add nsw i16 %storemerge, 1
  br label %for.cond

for.end:                                          ; preds = %for.cond
  ret void
}

Note the

for.body:                                         ; preds = %for.cond
  br i1 true, label %cond.false, label %land.rhs

land.rhs:                                         ; preds = %for.body
  br i1 poison, label %cond.end, label %cond.false

%land.rhs is dead due to the "br i1 true" branch, but I think it still is involved somehow in making the stored value poison with this patch.

Oh, thanks. I'll look into this.

bjope added a subscriber: bjope.Jan 26 2021, 4:30 AM
bjope added inline comments.
/llvm/lib/IR/Constants.cpp
1344

@aqjune Shouldn't this check include isPoison?

Now you will return a poison value if the first element in the aggregate is poison (not really checking if all elements are poison as the desciption says).

aqjune added inline comments.Jan 26 2021, 6:43 AM
/llvm/lib/IR/Constants.cpp
1344

PoisonValue is a subclass of UndefValue, so isUndef already checks that. :)

bjope added inline comments.Jan 26 2021, 7:19 AM
/llvm/lib/IR/Constants.cpp
1344

Ok. I didn't know that.

But maybe there should have been a test case for that scenario in https://reviews.llvm.org/rGc95f39891a282ebf36199c73b705d4a2c78a46ce
Also verifying

Constant *CUP = ConstantVector::get({CP, CU});

(seemed a bit weird that you tested three out of four permutations, missing the one hidden in that subclass dependency)