Page MenuHomePhabricator

Fix PR45371: SeparateConstOffsetFromGEP clean up bookkeeping
ClosedPublic

Authored by jroelofs on Mar 31 2020, 12:23 PM.

Details

Summary

find() was altering the UserChain, even in cases where it subsequently
discovered that the resulting constant was a 0. This confuses
rebuildWithoutConstOffset() when it attempts to walk the chain later, since it
is expected that the chain itself be a path down the use-def edges of an
expression.

Diff Detail

Event Timeline

jroelofs created this revision.Mar 31 2020, 12:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2020, 12:23 PM
bjope added a subscriber: bjope.Mar 31 2020, 12:41 PM
bjope added inline comments.
llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp
580

Coding style: You may skip the braces here.

ajwock accepted this revision.Mar 31 2020, 1:47 PM

LGTM, just give the test a good name.

llvm/test/Transforms/SeparateConstOffsetFromGEP/pr45371.ll
6 ↗(On Diff #253962)

Can we have a name for this file and this function that's a little more self explanatory? If it weren't attached to this commit I would have no idea what's being tested.

This revision is now accepted and ready to land.Mar 31 2020, 1:47 PM
jroelofs marked 2 inline comments as done.Mar 31 2020, 3:21 PM
jroelofs added inline comments.
llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp
580

Haven't seen (m)any braceless if's with a comment inside them. Happy to reword and drop them though.

llvm/test/Transforms/SeparateConstOffsetFromGEP/pr45371.ll
6 ↗(On Diff #253962)

It's not uncommon to name tests after their corresponding bugzilla number:

$ find llvm/test/ -name "pr[0-9]*.ll" | wc -l
     791

Suggestions on a better function name?

jroelofs updated this revision to Diff 254029.Mar 31 2020, 3:52 PM

implement review feedback

Thanks! This indeed seem to solve the problem I saw.