Page MenuHomePhabricator

[InstCombine] Use m_Zero instead of isNullValue() when checking if a GEP index is all zeroes to prevent an infinite loop.
ClosedPublic

Authored by craig.topper on Sep 24 2019, 12:42 PM.

Details

Summary

The test case here previously infinite looped. Only one element from the GEP is used so SimplifyDemandedVectorElts would replace the other lanes in each index with undef leading to the first index being <0, undef, undef, undef>. But there's a GEP transform that tries to replace an index into a 0 sized type with a zero index. But the zero index check only works on ConstantInt 0 or ConstantAggregateZero so it would turn the index back to zeroinitializer. Resulting in a loop.

The fix is to use m_Zero() to allow a vector of zeroes and undefs.

Diff Detail

Event Timeline

craig.topper created this revision.Sep 24 2019, 12:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 24 2019, 12:43 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

I'm fine with this landing as a workaround, but it really feels like the right fix is on the other transform. <0, undef> is effectively a zero index (or can be chosen as such), and our transforms should be aware of that.

More generally, I'm increasingly thinking the implicit broadcast feature of vector geps is a mistake. I'm tempted to just change the LangRef and require a separate explicit broadcast via a shuffle.

Maybe there's something in PatternMatch we can use that allows undef?

I haven't stepped through the example, but m_Zero() and m_ZeroInt() allow undefs.

craig.topper retitled this revision from [InstCombine] Don't apply SimplifyDemandedVectorElts to ConstantAggregateZero GEP indices to [InstCombine] Use m_Zero instead of isNullValue() when checking if a GEP index is all zeroes to prevent an infinite loop..
craig.topper edited the summary of this revision. (Show Details)
spatel accepted this revision.Sep 26 2019, 9:23 AM

LGTM

This revision is now accepted and ready to land.Sep 26 2019, 9:23 AM
This revision was automatically updated to reflect the committed changes.