Skip to content

Commit 3a0a0c2

Browse files
artagnonartagnon
committed
VT: don't create undef in isBytewiseValue
The original motivation for this patch derives from a correctness issue found in MemCpyOpt's fca2memcpy.ll test by Alive2. The output from Alive2 is reproduced below: ---------------------------------------- define void @addrproducer(ptr %src, ptr %dst) { %#1 = load {ptr, i8, i24, i32}, ptr %src, align 8 store {ptr, i8, i24, i32} undef, ptr %dst, align 8 %dst2 = gep ptr %dst, 16 x i64 1 store {ptr, i8, i24, i32} %#1, ptr %dst2, align 8 ret void } => define void @addrproducer(ptr %src, ptr %dst) { %dst2 = gep ptr %dst, 16 x i64 1 memmove ptr %dst2 align 8, ptr %src align 8, i64 16 memset ptr %dst align 8, i8 undef, i64 16 ret void } Transformation doesn't verify! (unsound) ERROR: Mismatch in memory Example: ptr %src = pointer(non-local, block_id=1, offset=0) / Address=#x08 ptr %dst = pointer(non-local, block_id=1, offset=0) / Address=#x08 Source: {ptr, i8, i24, i32} %#1 = { poison, poison, poison, poison } ptr %dst2 = pointer(non-local, block_id=1, offset=16) / Address=#x18 SOURCE MEMORY STATE =================== NON-LOCAL BLOCKS: Block 0 > size: 0 align: 1 alloc type: 0 alive: false address: 0 Block 1 > size: 71 align: 1 alloc type: 0 alive: true address: 8 Block 2 > size: 0 align: 2 alloc type: 0 alive: true address: 4 Block 3 > size: 0 align: 1 alloc type: 0 alive: true address: 4 Target: ptr %dst2 = pointer(non-local, block_id=1, offset=16) / Address=#x18 Mismatch in pointer(non-local, block_id=1, offset=0) Source value: null, byte offset=0 Target value: #x01 ---------------------------------------- The underlying problem is in llvm::isBytewiseValue(), which creates and returns an UndefValue when called with an UndefValue, and this result is used by MemCpyOpt, leading to an incorrect optimization. Generally speaking, reasoning about undef values when optimizing is tricky, and considering that undef is scheduled for removal, change the function to bail out on undef values, handle poison instead, and create a poison value for all other purposes. Auding the callers of this function reveals that MemCpyOpt is the only caller that explicitly checks the return value of this function against undef: change this as well. This patch has the nice side-effect of cleaning up some undefs in the tests of MemCpyOpt and the unittests of ValueTracking.
1 parent cb4433b commit 3a0a0c2

File tree

7 files changed

+66
-63
lines changed

7 files changed

+66
-63
lines changed

‎llvm/include/llvm/Analysis/ValueTracking.h‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -617,8 +617,7 @@ inline std::optional<bool> computeKnownFPSignBit(const Value *V, unsigned Depth,
617617
/// return the i8 value that it is represented with. This is true for all i8
618618
/// values obviously, but is also true for i32 0, i32 -1, i16 0xF0F0, double
619619
/// 0.0 etc. If the value can't be handled with a repeated byte store (e.g.
620-
/// i16 0x1234), return null. If the value is entirely undef and padding,
621-
/// return undef.
620+
/// i16 0x1234), return null. If the value is undef, also return null.
622621
Value *isBytewiseValue(Value *V, const DataLayout &DL);
623622

624623
/// Given an aggregate and an sequence of indices, see if the scalar value

‎llvm/lib/Analysis/ValueTracking.cpp‎

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6163,14 +6163,18 @@ Value *llvm::isBytewiseValue(Value *V, const DataLayout &DL) {
61636163

61646164
LLVMContext &Ctx = V->getContext();
61656165

6166-
// Undef don't care.
6167-
auto *UndefInt8 = UndefValue::get(Type::getInt8Ty(Ctx));
6166+
// Allow poison.
6167+
auto *Poison = PoisonValue::get(Type::getInt8Ty(Ctx));
6168+
if (isa<PoisonValue>(V))
6169+
return Poison;
6170+
6171+
// Forbid optimization over undef, for correctness reasons.
61686172
if (isa<UndefValue>(V))
6169-
return UndefInt8;
6173+
return nullptr;
61706174

6171-
// Return Undef for zero-sized type.
6175+
// Return poison for zero-sized type.
61726176
if (DL.getTypeStoreSize(V->getType()).isZero())
6173-
return UndefInt8;
6177+
return Poison;
61746178

61756179
Constant *C = dyn_cast<Constant>(V);
61766180
if (!C) {
@@ -6228,23 +6232,23 @@ Value *llvm::isBytewiseValue(Value *V, const DataLayout &DL) {
62286232
return LHS;
62296233
if (!LHS || !RHS)
62306234
return nullptr;
6231-
if (LHS == UndefInt8)
6235+
if (LHS == Poison)
62326236
return RHS;
6233-
if (RHS == UndefInt8)
6237+
if (RHS == Poison)
62346238
return LHS;
62356239
return nullptr;
62366240
};
62376241

62386242
if (ConstantDataSequential *CA = dyn_cast<ConstantDataSequential>(C)) {
6239-
Value *Val = UndefInt8;
6243+
Value *Val = Poison;
62406244
for (unsigned I = 0, E = CA->getNumElements(); I != E; ++I)
62416245
if (!(Val = Merge(Val, isBytewiseValue(CA->getElementAsConstant(I), DL))))
62426246
return nullptr;
62436247
return Val;
62446248
}
62456249

62466250
if (isa<ConstantAggregate>(C)) {
6247-
Value *Val = UndefInt8;
6251+
Value *Val = Poison;
62486252
for (Value *Op : C->operands())
62496253
if (!(Val = Merge(Val, isBytewiseValue(Op, DL))))
62506254
return nullptr;

‎llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -418,8 +418,8 @@ Instruction *MemCpyOptPass::tryMergingIntoMemset(Instruction *StartInst,
418418

419419
// Check to see if this stored value is of the same byte-splattable value.
420420
Value *StoredByte = isBytewiseValue(StoredVal, DL);
421-
if (isa<UndefValue>(ByteVal) && StoredByte)
422-
ByteVal = StoredByte;
421+
if (!StoredByte)
422+
break;
423423
if (ByteVal != StoredByte)
424424
break;
425425

‎llvm/test/Transforms/MemCpyOpt/fca2memcpy.ll‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,8 @@ define void @destroysrc(ptr %src, ptr %dst) {
5151

5252
define void @destroynoaliassrc(ptr noalias %src, ptr %dst) {
5353
; CHECK-LABEL: @destroynoaliassrc(
54-
; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i64(ptr align 8 [[DST:%.*]], ptr align 8 [[SRC]], i64 16, i1 false)
55-
; CHECK-NEXT: call void @llvm.memset.p0.i64(ptr align 8 [[SRC:%.*]], i8 0, i64 16, i1 false)
54+
; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i64(ptr align 8 [[DST:%.*]], ptr align 8 [[SRC:%.*]], i64 16, i1 false)
55+
; CHECK-NEXT: call void @llvm.memset.p0.i64(ptr align 8 [[SRC]], i8 0, i64 16, i1 false)
5656
; CHECK-NEXT: ret void
5757
;
5858
%1 = load %S, ptr %src
@@ -79,13 +79,13 @@ define void @copyalias(ptr %src, ptr %dst) {
7979
; sure we lift the computation as well if needed and possible.
8080
define void @addrproducer(ptr %src, ptr %dst) {
8181
; CHECK-LABEL: @addrproducer(
82-
; CHECK-NEXT: [[DST2:%.*]] = getelementptr [[S:%.*]], ptr [[DST]], i64 1
82+
; CHECK-NEXT: [[DST2:%.*]] = getelementptr [[S:%.*]], ptr [[DST:%.*]], i64 1
8383
; CHECK-NEXT: call void @llvm.memmove.p0.p0.i64(ptr align 8 [[DST2]], ptr align 8 [[SRC:%.*]], i64 16, i1 false)
84-
; CHECK-NEXT: call void @llvm.memset.p0.i64(ptr align 8 [[DST:%.*]], i8 undef, i64 16, i1 false)
84+
; CHECK-NEXT: call void @llvm.memset.p0.i64(ptr align 8 [[DST]], i8 poison, i64 16, i1 false)
8585
; CHECK-NEXT: ret void
8686
;
8787
%1 = load %S, ptr %src
88-
store %S undef, ptr %dst
88+
store %S poison, ptr %dst
8989
%dst2 = getelementptr %S , ptr %dst, i64 1
9090
store %S %1, ptr %dst2
9191
ret void
@@ -94,14 +94,14 @@ define void @addrproducer(ptr %src, ptr %dst) {
9494
define void @aliasaddrproducer(ptr %src, ptr %dst, ptr %dstidptr) {
9595
; CHECK-LABEL: @aliasaddrproducer(
9696
; CHECK-NEXT: [[TMP1:%.*]] = load [[S:%.*]], ptr [[SRC:%.*]], align 8
97-
; CHECK-NEXT: call void @llvm.memset.p0.i64(ptr align 8 [[DST:%.*]], i8 undef, i64 16, i1 false)
97+
; CHECK-NEXT: call void @llvm.memset.p0.i64(ptr align 8 [[DST:%.*]], i8 poison, i64 16, i1 false)
9898
; CHECK-NEXT: [[DSTINDEX:%.*]] = load i32, ptr [[DSTIDPTR:%.*]], align 4
9999
; CHECK-NEXT: [[DST2:%.*]] = getelementptr [[S]], ptr [[DST]], i32 [[DSTINDEX]]
100100
; CHECK-NEXT: store [[S]] [[TMP1]], ptr [[DST2]], align 8
101101
; CHECK-NEXT: ret void
102102
;
103103
%1 = load %S, ptr %src
104-
store %S undef, ptr %dst
104+
store %S poison, ptr %dst
105105
%dstindex = load i32, ptr %dstidptr
106106
%dst2 = getelementptr %S , ptr %dst, i32 %dstindex
107107
store %S %1, ptr %dst2
@@ -113,12 +113,12 @@ define void @noaliasaddrproducer(ptr %src, ptr noalias %dst, ptr noalias %dstidp
113113
; CHECK-NEXT: [[TMP2:%.*]] = load i32, ptr [[DSTIDPTR:%.*]], align 4
114114
; CHECK-NEXT: [[DSTINDEX:%.*]] = or i32 [[TMP2]], 1
115115
; CHECK-NEXT: [[DST2:%.*]] = getelementptr [[S:%.*]], ptr [[DST:%.*]], i32 [[DSTINDEX]]
116-
; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i64(ptr align 8 [[DST2]], ptr align 8 [[SRC]], i64 16, i1 false)
117-
; CHECK-NEXT: call void @llvm.memset.p0.i64(ptr align 8 [[SRC:%.*]], i8 undef, i64 16, i1 false)
116+
; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i64(ptr align 8 [[DST2]], ptr align 8 [[SRC:%.*]], i64 16, i1 false)
117+
; CHECK-NEXT: call void @llvm.memset.p0.i64(ptr align 8 [[SRC]], i8 poison, i64 16, i1 false)
118118
; CHECK-NEXT: ret void
119119
;
120120
%1 = load %S, ptr %src
121-
store %S undef, ptr %src
121+
store %S poison, ptr %src
122122
%2 = load i32, ptr %dstidptr
123123
%dstindex = or i32 %2, 1
124124
%dst2 = getelementptr %S , ptr %dst, i32 %dstindex
@@ -130,7 +130,7 @@ define void @throwing_call(ptr noalias %src, ptr %dst) {
130130
; CHECK-LABEL: @throwing_call(
131131
; CHECK-NEXT: [[TMP1:%.*]] = load [[S:%.*]], ptr [[SRC:%.*]], align 8
132132
; CHECK-NEXT: call void @llvm.memset.p0.i64(ptr align 8 [[SRC]], i8 0, i64 16, i1 false)
133-
; CHECK-NEXT: call void @call() [[ATTR2:#.*]]
133+
; CHECK-NEXT: call void @call() #[[ATTR2:[0-9]+]]
134134
; CHECK-NEXT: store [[S]] [[TMP1]], ptr [[DST:%.*]], align 8
135135
; CHECK-NEXT: ret void
136136
;

‎llvm/test/Transforms/MemCpyOpt/memcpy-to-memset.ll‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,15 +3,15 @@
33

44
declare void @llvm.memcpy.p0.p0.i64(ptr nocapture, ptr nocapture, i64, i1) nounwind
55

6-
@undef = internal constant i32 undef, align 4
7-
define void @test_undef() nounwind {
8-
; CHECK-LABEL: @test_undef(
6+
@poison = internal constant i32 poison, align 4
7+
define void @test_poison() nounwind {
8+
; CHECK-LABEL: @test_poison(
99
; CHECK-NEXT: [[A:%.*]] = alloca i32, align 4
10-
; CHECK-NEXT: call void @llvm.memset.p0.i64(ptr align 4 [[A]], i8 undef, i64 4, i1 false)
10+
; CHECK-NEXT: call void @llvm.memset.p0.i64(ptr align 4 [[A]], i8 poison, i64 4, i1 false)
1111
; CHECK-NEXT: ret void
1212
;
1313
%a = alloca i32, align 4
14-
call void @llvm.memcpy.p0.p0.i64(ptr align 4 %a, ptr align 4 @undef, i64 4, i1 false)
14+
call void @llvm.memcpy.p0.p0.i64(ptr align 4 %a, ptr align 4 @poison, i64 4, i1 false)
1515
ret void
1616
}
1717

@@ -27,15 +27,15 @@ define void @test_i32x3() nounwind {
2727
ret void
2828
}
2929

30-
@i32x3_undef = internal constant [3 x i32] [i32 -1, i32 undef, i32 -1], align 4
31-
define void @test_i32x3_undef() nounwind {
32-
; CHECK-LABEL: @test_i32x3_undef(
30+
@i32x3_poison = internal constant [3 x i32] [i32 -1, i32 poison, i32 -1], align 4
31+
define void @test_i32x3_poison() nounwind {
32+
; CHECK-LABEL: @test_i32x3_poison(
3333
; CHECK-NEXT: [[A:%.*]] = alloca [3 x i32], align 4
3434
; CHECK-NEXT: call void @llvm.memset.p0.i64(ptr align 4 [[A]], i8 -1, i64 12, i1 false)
3535
; CHECK-NEXT: ret void
3636
;
3737
%a = alloca [3 x i32], align 4
38-
call void @llvm.memcpy.p0.p0.i64(ptr align 4 %a, ptr align 4 @i32x3_undef, i64 12, i1 false)
38+
call void @llvm.memcpy.p0.p0.i64(ptr align 4 %a, ptr align 4 @i32x3_poison, i64 12, i1 false)
3939
ret void
4040
}
4141

‎llvm/test/Transforms/MemCpyOpt/store-to-memset-is-nonzero-type.ll‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55

66
define void @array_zero(ptr %p) {
77
; CHECK-LABEL: @array_zero(
8-
; CHECK-NEXT: call void @llvm.memset.p0.i64(ptr align 1 [[P:%.*]], i8 undef, i64 0, i1 false)
8+
; CHECK-NEXT: call void @llvm.memset.p0.i64(ptr align 1 [[P:%.*]], i8 poison, i64 0, i1 false)
99
; CHECK-NEXT: ret void
1010
;
1111
store [0 x i8] zeroinitializer, ptr %p
@@ -25,7 +25,7 @@ define void @array_nonzero(ptr %p) {
2525

2626
define void @struct_zero(ptr %p) {
2727
; CHECK-LABEL: @struct_zero(
28-
; CHECK-NEXT: call void @llvm.memset.p0.i64(ptr align 1 [[P:%.*]], i8 undef, i64 0, i1 false)
28+
; CHECK-NEXT: call void @llvm.memset.p0.i64(ptr align 1 [[P:%.*]], i8 poison, i64 0, i1 false)
2929
; CHECK-NEXT: ret void
3030
;
3131
store { } zeroinitializer, ptr %p

‎llvm/unittests/Analysis/ValueTrackingTest.cpp‎

Lines changed: 28 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -2769,8 +2769,8 @@ const std::pair<const char *, const char *> IsBytewiseValueTests[] = {
27692769
"ptr null",
27702770
},
27712771
{
2772-
"i8 undef",
2773-
"ptr undef",
2772+
"i8 poison",
2773+
"ptr poison",
27742774
},
27752775
{
27762776
"i8 0",
@@ -2789,8 +2789,8 @@ const std::pair<const char *, const char *> IsBytewiseValueTests[] = {
27892789
"i8 -1",
27902790
},
27912791
{
2792-
"i8 undef",
2793-
"i16 undef",
2792+
"i8 poison",
2793+
"i16 poison",
27942794
},
27952795
{
27962796
"i8 0",
@@ -2869,28 +2869,28 @@ const std::pair<const char *, const char *> IsBytewiseValueTests[] = {
28692869
"ptr inttoptr (i96 -1 to ptr)",
28702870
},
28712871
{
2872-
"i8 undef",
2872+
"i8 poison",
28732873
"[0 x i8] zeroinitializer",
28742874
},
28752875
{
2876-
"i8 undef",
2877-
"[0 x i8] undef",
2876+
"i8 poison",
2877+
"[0 x i8] poison",
28782878
},
28792879
{
2880-
"i8 undef",
2880+
"i8 poison",
28812881
"[5 x [0 x i8]] zeroinitializer",
28822882
},
28832883
{
2884-
"i8 undef",
2885-
"[5 x [0 x i8]] undef",
2884+
"i8 poison",
2885+
"[5 x [0 x i8]] poison",
28862886
},
28872887
{
28882888
"i8 0",
28892889
"[6 x i8] zeroinitializer",
28902890
},
28912891
{
2892-
"i8 undef",
2893-
"[6 x i8] undef",
2892+
"i8 poison",
2893+
"[6 x i8] poison",
28942894
},
28952895
{
28962896
"i8 1",
@@ -2910,15 +2910,15 @@ const std::pair<const char *, const char *> IsBytewiseValueTests[] = {
29102910
},
29112911
{
29122912
"i8 1",
2913-
"[4 x i8] [i8 1, i8 undef, i8 1, i8 1]",
2913+
"[4 x i8] [i8 1, i8 poison, i8 1, i8 1]",
29142914
},
29152915
{
29162916
"i8 0",
29172917
"<6 x i8> zeroinitializer",
29182918
},
29192919
{
2920-
"i8 undef",
2921-
"<6 x i8> undef",
2920+
"i8 poison",
2921+
"<6 x i8> poison",
29222922
},
29232923
{
29242924
"i8 1",
@@ -2938,15 +2938,15 @@ const std::pair<const char *, const char *> IsBytewiseValueTests[] = {
29382938
},
29392939
{
29402940
"i8 5",
2941-
"<2 x i8> < i8 5, i8 undef >",
2941+
"<2 x i8> < i8 5, i8 poison >",
29422942
},
29432943
{
29442944
"i8 0",
29452945
"[2 x [2 x i16]] zeroinitializer",
29462946
},
29472947
{
2948-
"i8 undef",
2949-
"[2 x [2 x i16]] undef",
2948+
"i8 poison",
2949+
"[2 x [2 x i16]] poison",
29502950
},
29512951
{
29522952
"i8 -86",
@@ -2959,36 +2959,36 @@ const std::pair<const char *, const char *> IsBytewiseValueTests[] = {
29592959
"[2 x i16] [i16 -21836, i16 -21846]]",
29602960
},
29612961
{
2962-
"i8 undef",
2962+
"i8 poison",
29632963
"{ } zeroinitializer",
29642964
},
29652965
{
2966-
"i8 undef",
2967-
"{ } undef",
2966+
"i8 poison",
2967+
"{ } poison",
29682968
},
29692969
{
2970-
"i8 undef",
2970+
"i8 poison",
29712971
"{ {}, {} } zeroinitializer",
29722972
},
29732973
{
2974-
"i8 undef",
2975-
"{ {}, {} } undef",
2974+
"i8 poison",
2975+
"{ {}, {} } poison",
29762976
},
29772977
{
29782978
"i8 0",
29792979
"{i8, i64, ptr} zeroinitializer",
29802980
},
29812981
{
2982-
"i8 undef",
2983-
"{i8, i64, ptr} undef",
2982+
"i8 poison",
2983+
"{i8, i64, ptr} poison",
29842984
},
29852985
{
29862986
"i8 -86",
2987-
"{i8, i64, ptr} {i8 -86, i64 -6148914691236517206, ptr undef}",
2987+
"{i8, i64, ptr} {i8 -86, i64 -6148914691236517206, ptr poison}",
29882988
},
29892989
{
29902990
"",
2991-
"{i8, i64, ptr} {i8 86, i64 -6148914691236517206, ptr undef}",
2991+
"{i8, i64, ptr} {i8 86, i64 -6148914691236517206, ptr poison}",
29922992
},
29932993
};
29942994

0 commit comments

Comments
 (0)