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

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

aqjune

Jan 6 2021, 4:26 PM

Parents

rG8deaec122ec6: [analyzer] Update Fuchsia checker to catch releasing unowned handles.

Branches

Unknown

Tags

Unknown

Event Timeline

aqjune committed rG8871a4b4cab8: [Constant] Update ConstantVector::get to return poison if all input elems are… (authored by aqjune).Jan 6 2021, 4:26 PM

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.

bbi-51947.ll1 KBDownload

I wrote a PR about this:
https://bugs.llvm.org/show_bug.cgi?id=48832

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)