Page MenuHomePhabricator

[InstCombine] Fix a vector-of-pointers instcombine undef bug.
Needs ReviewPublic

Authored by sheredom on Apr 12 2019, 1:56 AM.

Details

Summary

This commit fixes a really fun bug with inst combine where you have a
vector-of-pointers and only one element of this vector is used.
InstCombine correctly notes that an element is not used, and then will
go back through a gep into that vector-of-pointers and set all vector
elements to undef. This is fine in all cases except that the langref
(and a ton of places in the optimizer) requires that indexing into a
struct has the same index for each element of the vector-of-pointers.

To fix the bug I've just made the gep/undef propagation check that the
current type being indexed into is not a struct when doing the undef
propagation.

Diff Detail

Event Timeline

sheredom created this revision.Apr 12 2019, 1:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2019, 1:56 AM

We clearly need to fix the bug (crash). @reames or someone else with more GEP experience should have a look though. I've just pointed out some minor bits to clean up.

lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
1177

nit: variables/parameters should (unfortunately) be capitalized, so:
i -> Index
isIndexStruct -> IsIndexStruct
?

Also, don't need to explicitly state the return type?

1193

element this -> element of this?

1206–1208

No need for braces here.

1211–1215

No need for braces here.

test/Transforms/InstCombine/vector-of-pointers.ll
1–4 ↗(On Diff #194814)

I would prefer to put this test in the same file that D57468 used since that was where the bug was introduced:
llvm/trunk/test/Transforms/InstCombine/vec_demanded_elts.ll

Also, please use this script to automatically create/update the CHECK lines (that makes it easier to update in the future):
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py

reames requested changes to this revision.Apr 17 2019, 1:59 PM

I think you might be overcomplicating this. I'd suggest the following:

  1. scan all index types for a gep
  2. run current code, but if any operand was gep, skip the recursive call on the operand

It's slightly less powerful, but correct, and easy. And frankly, optimize geps of struct types is probably rare enough we don't care.

p.s. Agreed on the location of test file, please add to existing tests.

This revision now requires changes to proceed.Apr 17 2019, 1:59 PM

I think you might be overcomplicating this. I'd suggest the following:

  1. scan all index types for a gep
  2. run current code, but if any operand was gep, skip the recursive call on the operand

    It's slightly less powerful, but correct, and easy. And frankly, optimize geps of struct types is probably rare enough we don't care.

    p.s. Agreed on the location of test file, please add to existing tests.

I don't really understand what your suggestion is here sorry - let me try and rephrase the suggestion and see if we agree?

  1. You are saying that vector-of-pointers-to-struct is something that is rarely seen.
  2. Your suggestion to fix this issue is to basically bail out of doing the get element ptr recursive simplify call if we see a vector-of-pointers-to-struct?

For 1., the reason I filed this bug is we have a bunch of code that has vector-of-pointers-to-struct, we use this to handle row-major matrices in our graphics stack. I want us to be as optimal as possible in this case, which leads me into 2. - is there any strong reason why we shouldn't do the more powerful thing here? EG. is there something intrinsically wrong/fragile with my approach that worries you going forward?

sheredom updated this revision to Diff 196205.Apr 23 2019, 3:41 AM

Fix some review comments by @spatel

sheredom marked 4 inline comments as done.Apr 23 2019, 3:41 AM
spatel added a subscriber: RKSimon.Tue, Apr 30, 5:06 AM

A fuzzer found what looks like the same bug in PR41624:
https://bugs.llvm.org/show_bug.cgi?id=41624

@sheredom - can you confirm? (and may want to add this minimal test to the patch for more coverage)

define i32* @PR41624(<2 x { i32, i32 }*> %a) {
  %w = getelementptr { i32, i32 }, <2 x { i32, i32 }*> %a, <2 x i64> <i64 5, i64 undef>, <2 x i32> <i32 0, i32 0>
  %r = extractelement <2 x i32*> %w, i1 0
  ret i32* %r
}

@spatel yup its the same bug, and my fix fixes it to. I'll add it to my test changes.

sheredom updated this revision to Diff 197310.Tue, Apr 30, 6:25 AM

Added the extra test case that @spatel found that my change also fixes.

I think you might be overcomplicating this. I'd suggest the following:

  1. scan all index types for a gep
  2. run current code, but if any operand was gep, skip the recursive call on the operand

    It's slightly less powerful, but correct, and easy. And frankly, optimize geps of struct types is probably rare enough we don't care.

    p.s. Agreed on the location of test file, please add to existing tests.

I don't really understand what your suggestion is here sorry - let me try and rephrase the suggestion and see if we agree?

I submitted a version along what I proposed as https://reviews.llvm.org/rL359633. Please confirm that fixes your crash. If you want to work on a better version of the fix, I'm happy to review, but I wanted to make sure the crash was resolved.

As noted in the bug, I think the "right" fix here is to change the LangRef.

Requiring integer constant indices just based on the type being indexed seems to be a bad idea, and without some strong justification, I'd argue should simply be removed. If you want such geps to be better optimized, making hem behave like all other geps is better than special casing any particular transform. (Like this one.) To be clear, I'm suggesting allowing arbitrary constants as operands to said geps, not arbitrary values.

I think you might be overcomplicating this. I'd suggest the following:

  1. scan all index types for a gep
  2. run current code, but if any operand was gep, skip the recursive call on the operand

    It's slightly less powerful, but correct, and easy. And frankly, optimize geps of struct types is probably rare enough we don't care.

    p.s. Agreed on the location of test file, please add to existing tests.

I don't really understand what your suggestion is here sorry - let me try and rephrase the suggestion and see if we agree?

I submitted a version along what I proposed as https://reviews.llvm.org/rL359633. Please confirm that fixes your crash. If you want to work on a better version of the fix, I'm happy to review, but I wanted to make sure the crash was resolved.

I can confirm your fix does fix my crash.

I guess by 'better version of the fix' you mean relaxing the langref and dealing with the fallout? The only intermediate fix would be what this review originally proposed.

Is there any way to get the history of why the lang ref had the struct-index restriction it did? My only notion is that it is to ensure that each element of the vector-of-pointers had the same 'depth' of GEP.