This is an archive of the discontinued LLVM Phabricator instance.

Differential D40231

InstructionSimplify: 'extractelement' with an undef index is undef
ClosedPublic

Authored by zvi on Nov 19 2017, 3:01 PM.

Details

Reviewers
arsenm
spatel
majnemer
Commits
rG2e6e88f6890b: InstructionSimplify: 'extractelement' with an undef index is undef
rL319910: InstructionSimplify: 'extractelement' with an undef index is undef
Summary

An undef extract index can be arbitrarily chosen to be an
out-of-range index value, which would result in the instruction being undef.

This change closes a gap identified while working on lowering vector permute intrinsics
with variable index vectors to pure LLVM IR.

Diff Detail

Repository
rL LLVM

Event Timeline

zvi created this revision.Nov 19 2017, 3:01 PM
Herald added a subscriber: wdng. · View Herald TranscriptNov 19 2017, 3:01 PM
zvi added a comment.Nov 19 2017, 3:05 PM

Hi @arsenm,

With this patch i am seeing failures in test/CodeGen/AMDGPU/indirect-addressing-si.ll . This test include extractelement instructions with undef indices, which is what this patch targets. I would appreciate your help with advice on how we can modify these tests so that they can be used safely. Please bear in mind that i have no knowledge of the AMDGPU backend.

Thanks, Zvi

arsenm edited edge metadata.Nov 20 2017, 4:45 PM
In D40231#929842, @zvi wrote:

Hi @arsenm,

With this patch i am seeing failures in test/CodeGen/AMDGPU/indirect-addressing-si.ll . This test include extractelement instructions with undef indices, which is what this patch targets. I would appreciate your help with advice on how we can modify these tests so that they can be used safely. Please bear in mind that i have no knowledge of the AMDGPU backend.

Thanks, Zvi

I think the point of the tests was to make sure we don't hit use of undefined register errors when it's expanded . I don't think InstructionSimplify is guaranteed during codegen, so it would be nice if there was still a way to get an undef index through codegen.

zvi added a comment.Nov 20 2017, 10:59 PM

One option is to run llc with -O0 which will disable passes which use InstructionSimplify, but this will require changing the CHECK's to expect different generated code. If the purpose of the test is to not crash in the Verifier, are the CHECK's needed?

arsenm added a comment.Nov 27 2017, 10:16 AM
In D40231#931175, @zvi wrote:

One option is to run llc with -O0 which will disable passes which use InstructionSimplify, but this will require changing the CHECK's to expect different generated code. If the purpose of the test is to not crash in the Verifier, are the CHECK's needed?

The checks probably aren't needed, but if this is eliminated in the IR the test isn't reaching the point it needs to. Is there another way to sneak an undef index in? i.e. some sequence that will fold to undef in the DAG that InstructionSimplify won't catch

spatel edited edge metadata.Nov 27 2017, 10:49 AM
In D40231#936224, @arsenm wrote:
In D40231#931175, @zvi wrote:

One option is to run llc with -O0 which will disable passes which use InstructionSimplify, but this will require changing the CHECK's to expect different generated code. If the purpose of the test is to not crash in the Verifier, are the CHECK's needed?

The checks probably aren't needed, but if this is eliminated in the IR the test isn't reaching the point it needs to. Is there another way to sneak an undef index in? i.e. some sequence that will fold to undef in the DAG that InstructionSimplify won't catch

Even if we find a loophole in InstSimplify, I think that would be a fragile solution (InstSimplify could get smarter and break the tests again). This could bite us again in the near future because over in D40390, I've suggested adding another simplification for 'insertelement' with an out-of-range index, and indirect-addressing-si.ll has tests that appear to rely on that not folding as well?

Running with -O0 seems like a good option here. Note that there's already a indirect-addressing-si-noopt.ll test file with a FIXME comment:

; FIXME: Merge into indirect-addressing-si.ll

...but maybe we should rearrange the tests so that things that are relying on undef are in that file?

spatel mentioned this in D40650: [InstSimplify] Fold insertelement into undef if index is out of bounds.Nov 30 2017, 7:33 AM
spatel mentioned this in D40390: [InstCombine] Don't crash on out of bounds index in the insertelement.Nov 30 2017, 8:32 AM
zvi updated this revision to Diff 125296.Dec 3 2017, 3:30 PM

Following Sanjay's suggestion, moving test-case from indirect-addressing-si.ll to indirect-addressing-si-noopt.ll.
If this looks ok i will commit the changes to these tests in a separate commit.

Herald added a subscriber: nhaehnle. · View Herald TranscriptDec 3 2017, 3:30 PM
Harbormaster completed remote builds in B12710: Diff 125296.Dec 3 2017, 3:30 PM
arsenm added inline comments.Dec 3 2017, 3:46 PM
test/CodeGen/AMDGPU/indirect-addressing-si-noopt.ll
24 ↗(On Diff #125296)

The checks were still dropped. Does -O0 prevent the inst simplify?

I really wish llc ran no IR optimizations, but we're stuck working around it

zvi added inline comments.Dec 3 2017, 10:41 PM
test/CodeGen/AMDGPU/indirect-addressing-si-noopt.ll
24 ↗(On Diff #125296)

llc -O0 should be running only IR passes required for functionality, so if InstSimplify is invoked, i would consider it as a bug.

fhahn added a subscriber: fhahn.Dec 4 2017, 1:11 AM
spatel added inline comments.Dec 4 2017, 7:31 AM
test/CodeGen/AMDGPU/indirect-addressing-si-noopt.ll
24 ↗(On Diff #125296)

I was curious how we get here, so I can confirm that -O0 prevents the instsimplify. Instsimplification is being invoked via early-cse which is being added by AMDGPUPassConfig::addEarlyCSEOrGVNPass(). It should be easy enough to add a cl::opt to customize that, but I don't think we should require that for this instsimplify patch to proceed.

zvi added inline comments.Dec 4 2017, 11:40 AM
test/CodeGen/AMDGPU/indirect-addressing-si-noopt.ll
24 ↗(On Diff #125296)

If the test is intended to cover post-isel handling of undef operands, consider replacing with a .mir test.

spatel accepted this revision.Dec 4 2017, 3:37 PM

LGTM. We have at least 2 suggestions to improve the AMDGPU tests, but we shouldn't need to hold up this patch for that fix.

This revision is now accepted and ready to land.Dec 4 2017, 3:37 PM
arsenm added inline comments.Dec 5 2017, 9:47 AM
test/CodeGen/AMDGPU/indirect-addressing-si-noopt.ll
24 ↗(On Diff #125296)

A MIR test doesn't make sense because this is supposed to test the initial MIR emission

arsenm added inline comments.Dec 5 2017, 9:52 AM
test/CodeGen/AMDGPU/indirect-addressing-si-noopt.ll
24 ↗(On Diff #125296)

It looks like that is disabled for -O0, and I would expect the check lines to work as-is for -O0

zvi updated this revision to Diff 125603.Dec 5 2017, 1:45 PM

Preseve migrated test-case's CHECK:'s which as @arnsenm pointed out can used as-is for -O0.

Harbormaster completed remote builds in B12772: Diff 125603.Dec 5 2017, 1:45 PM
zvi marked an inline comment as done.Dec 5 2017, 1:47 PM
arsenm accepted this revision.Dec 5 2017, 3:52 PM

LGTM

zvi mentioned this in rL319907: AMDGPU Tests: Change a case to be run with -O0.Dec 6 2017, 9:40 AM
zvi updated this revision to Diff 125758.Dec 6 2017, 9:50 AM

Final rebase

Harbormaster completed remote builds in B12819: Diff 125758.Dec 6 2017, 9:50 AM
Closed by commit rL319910: InstructionSimplify: 'extractelement' with an undef index is undef (authored by zvi). · Explain WhyDec 6 2017, 9:52 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
llvm/
trunk/
lib/
Analysis/
5 lines
test/
Transforms/
InstSimplify/
7 lines

Diff 125760

llvm/trunk/lib/Analysis/InstructionSimplify.cpp

Show First 20 Lines • Show All 3,895 Lines • ▼ Show 20 Linesstatic Value *SimplifyExtractElementInst(Value *Vec, Value *Idx, const SimplifyQuery &,
} }
// If extracting a specified index from the vector, see if we can recursively // If extracting a specified index from the vector, see if we can recursively
// find a previously computed scalar that was inserted into the vector. // find a previously computed scalar that was inserted into the vector.
if (auto *IdxC = dyn_cast<ConstantInt>(Idx)) if (auto *IdxC = dyn_cast<ConstantInt>(Idx))
if (Value *Elt = findScalarElement(Vec, IdxC->getZExtValue())) if (Value *Elt = findScalarElement(Vec, IdxC->getZExtValue()))
return Elt; return Elt;
// An undef extract index can be arbitrarily chosen to be an out-of-range
// index value, which would result in the instruction being undef.
if (isa<UndefValue>(Idx))
return UndefValue::get(Vec->getType()->getVectorElementType());
return nullptr; return nullptr;
} }
Value *llvm::SimplifyExtractElementInst(Value *Vec, Value *Idx, Value *llvm::SimplifyExtractElementInst(Value *Vec, Value *Idx,
const SimplifyQuery &Q) { const SimplifyQuery &Q) {
return ::SimplifyExtractElementInst(Vec, Idx, Q, RecursionLimit); return ::SimplifyExtractElementInst(Vec, Idx, Q, RecursionLimit);
} }
▲ Show 20 Lines • Show All 977 Lines • Show Last 20 Lines

llvm/trunk/test/Transforms/InstSimplify/2011-09-05-InsertExtractValue.ll

Show All 40 Linesdefine i8 @test4(<8 x i8> %V) {
%add = add <8 x i8> %V, bitcast (double 0x319BEB8FD172E36 to <8 x i8>) %add = add <8 x i8> %V, bitcast (double 0x319BEB8FD172E36 to <8 x i8>)
%extract = extractelement <8 x i8> %add, i32 6 %extract = extractelement <8 x i8> %add, i32 6
ret i8 %extract ret i8 %extract
; CHECK-LABEL: @test4( ; CHECK-LABEL: @test4(
; CHECK: %[[add:.*]] = add <8 x i8> %V, bitcast (<1 x double> <double 0x319BEB8FD172E36> to <8 x i8>) ; CHECK: %[[add:.*]] = add <8 x i8> %V, bitcast (<1 x double> <double 0x319BEB8FD172E36> to <8 x i8>)
; CHECK-NEXT: %[[extract:.*]] = extractelement <8 x i8> %[[add]], i32 6 ; CHECK-NEXT: %[[extract:.*]] = extractelement <8 x i8> %[[add]], i32 6
; CHECK-NEXT: ret i8 %[[extract]] ; CHECK-NEXT: ret i8 %[[extract]]
} }
define i32 @test5(<4 x i32> %V) {
%extract = extractelement <4 x i32> %V, i32 undef
ret i32 %extract
}
; CHECK-LABEL: @test5(
; CHECK: ret i32 undef