This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

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.

Event Timeline

zvi created this revision.Nov 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?

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#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?

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.

arsenm added inline comments.Dec 3 2017, 3:46 PM
test/CodeGen/AMDGPU/indirect-addressing-si-noopt.ll
24

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

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

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

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

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

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.

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 updated this revision to Diff 125758.Dec 6 2017, 9:50 AM

Final rebase

This revision was automatically updated to reflect the committed changes.