Skip to content

Commit fec2cba

Browse files
authored
[InstCombine] Fix miscompile when folding a select into a masked load (#216730)
`visitSelectInst` folds: select(mask, masked.load(ptr, mask, PT), FV) into: masked.load(ptr, mask, FV) The replacement load was previously created at the select, effectively moving the memory access past any intervening instructions. If one of them writes the loaded memory, the replacement load reads the updated value instead of the original one. This was also observed downstream in [ispc/ispc#3891](ispc/ispc#3891). The fold was added in `eb8589987267`. The issue is labelled `regression:22`, so it affects LLVM 22.1 as well as current trunk. Create the replacement load at the original load's position and require `FV` to be available there. Otherwise, leave the select unchanged. Requiring `FV` to be available at the original load means the fold no longer fires when `FV` is computed between the load and the select. No existing `llvm/test/Transforms` checks change as a result of this restriction. Tests cover an intervening aliasing store and the case where `FV` is unavailable at the original load. They also guard against carrying over call-site attributes such as `range` and `noundef` when those attributes no longer apply. Fixes #215453
1 parent 796a186 commit fec2cba

2 files changed

Lines changed: 51 additions & 5 deletions

File tree

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5379,11 +5379,16 @@ Instruction *InstCombinerImpl::visitSelectInst(SelectInst &SI) {
53795379
if (match(TrueVal, m_OneUse(m_MaskedLoad(m_Value(MaskedLoadPtr),
53805380
m_Specific(CondVal), m_Value())))) {
53815381
auto *LoadInst = cast<IntrinsicInst>(TrueVal);
5382-
Instruction *In = Builder.CreateMaskedLoad(
5383-
TrueVal->getType(), MaskedLoadPtr,
5384-
LoadInst->getParamAlign(0).valueOrOne(), CondVal, FalseVal);
5385-
In->setAAMetadata(LoadInst->getAAMetadata());
5386-
return replaceInstUsesWith(SI, In);
5382+
// Keep the load at its original position to avoid crossing writes. The new
5383+
// passthrough must therefore be available there.
5384+
if (DT.dominates(FalseVal, LoadInst)) {
5385+
Builder.SetInsertPoint(LoadInst);
5386+
Instruction *In = Builder.CreateMaskedLoad(
5387+
TrueVal->getType(), MaskedLoadPtr,
5388+
LoadInst->getParamAlign(0).valueOrOne(), CondVal, FalseVal);
5389+
In->setAAMetadata(LoadInst->getAAMetadata());
5390+
return replaceInstUsesWith(SI, In);
5391+
}
53875392
}
53885393

53895394
// Canonicalize sign function ashr pattern: select (icmp slt X, 1), ashr X,

llvm/test/Transforms/InstCombine/select-masked_load.ll

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,46 @@ define <vscale x 4 x i32> @fold_sel_into_masked_load_drop_metadata(ptr %loc, <vs
169169
ret <vscale x 4 x i32> %sel
170170
}
171171

172+
; Keep the folded load before an intervening aliasing store.
173+
define <4 x float> @fold_sel_into_masked_load_aliasing_store(ptr %ptr, <4 x i1> %mask, <4 x float> %passthrough) {
174+
; CHECK-LABEL: @fold_sel_into_masked_load_aliasing_store(
175+
; CHECK-NEXT: [[SEL:%.*]] = call <4 x float> @llvm.masked.load.v4f32.p0(ptr align 4 [[PTR:%.*]], <4 x i1> [[MASK:%.*]], <4 x float> [[PASSTHROUGH:%.*]])
176+
; CHECK-NEXT: store <4 x float> [[PASSTHROUGH]], ptr [[PTR]], align 16
177+
; CHECK-NEXT: ret <4 x float> [[SEL]]
178+
;
179+
%load = call <4 x float> @llvm.masked.load.v4f32.p0(ptr %ptr, i32 4, <4 x i1> %mask, <4 x float> zeroinitializer)
180+
store <4 x float> %passthrough, ptr %ptr, align 16
181+
%sel = select <4 x i1> %mask, <4 x float> %load, <4 x float> %passthrough
182+
ret <4 x float> %sel
183+
}
184+
185+
; Do not fold when the new passthrough is unavailable at the old load.
186+
define <4 x float> @neg_fold_sel_into_masked_load_passthrough_after_load(ptr %ptr, <4 x i1> %mask, <4 x float> %a) {
187+
; CHECK-LABEL: @neg_fold_sel_into_masked_load_passthrough_after_load(
188+
; CHECK-NEXT: [[LOAD:%.*]] = call <4 x float> @llvm.masked.load.v4f32.p0(ptr align 4 [[PTR:%.*]], <4 x i1> [[MASK:%.*]], <4 x float> zeroinitializer)
189+
; CHECK-NEXT: [[PASSTHROUGH:%.*]] = fadd <4 x float> [[A:%.*]], [[A]]
190+
; CHECK-NEXT: [[SEL:%.*]] = select <4 x i1> [[MASK]], <4 x float> [[LOAD]], <4 x float> [[PASSTHROUGH]]
191+
; CHECK-NEXT: ret <4 x float> [[SEL]]
192+
;
193+
%load = call <4 x float> @llvm.masked.load.v4f32.p0(ptr %ptr, i32 4, <4 x i1> %mask, <4 x float> zeroinitializer)
194+
%passthrough = fadd <4 x float> %a, %a
195+
%sel = select <4 x i1> %mask, <4 x float> %load, <4 x float> %passthrough
196+
ret <4 x float> %sel
197+
}
198+
199+
; Do not copy result or passthrough attributes (range/noundef) to the new load.
200+
; Use the current intrinsic form because auto-upgrading the legacy form drops
201+
; these attributes before InstCombine.
202+
define <8 x i16> @fold_sel_into_masked_load_drop_attrs(ptr %ptr, <8 x i1> %mask, <8 x i16> %passthrough) {
203+
; CHECK-LABEL: @fold_sel_into_masked_load_drop_attrs(
204+
; CHECK-NEXT: [[SEL:%.*]] = call <8 x i16> @llvm.masked.load.v8i16.p0(ptr align 2 [[PTR:%.*]], <8 x i1> [[MASK:%.*]], <8 x i16> [[PASSTHROUGH:%.*]])
205+
; CHECK-NEXT: ret <8 x i16> [[SEL]]
206+
;
207+
%load = call range(i16 0, 2) <8 x i16> @llvm.masked.load.v8i16.p0(ptr align 2 %ptr, <8 x i1> %mask, <8 x i16> noundef zeroinitializer)
208+
%sel = select <8 x i1> %mask, <8 x i16> %load, <8 x i16> %passthrough
209+
ret <8 x i16> %sel
210+
}
211+
172212
!0 = !{!1, !1, i64 0}
173213
!1 = !{!"int", !2, i64 0}
174214
!2 = !{!"omnipotent char", !8, i64 0}
@@ -184,3 +224,4 @@ define <vscale x 4 x i32> @fold_sel_into_masked_load_drop_metadata(ptr %loc, <vs
184224
declare <8 x float> @llvm.masked.load.v8f32.p0(ptr, i32 immarg, <8 x i1>, <8 x float>)
185225
declare <4 x i32> @llvm.masked.load.v4i32.p0(ptr, i32 immarg, <4 x i1>, <4 x i32>)
186226
declare <4 x float> @llvm.masked.load.v4f32.p0(ptr, i32 immarg, <4 x i1>, <4 x float>)
227+
declare <8 x i16> @llvm.masked.load.v8i16.p0(ptr, <8 x i1>, <8 x i16>)

0 commit comments

Comments
 (0)