Page MenuHomePhabricator

[ValueTracking] Don't let isAddOfNonZero look at adds with a PHI node as input
ClosedPublic

Authored by craig.topper on May 12 2017, 9:32 AM.

Details

Summary

This is a fix for the test case in PR32314.

Basic Alias Analysis can ask if two nodes are known non-equal after looking through a phi node to find a GEP. isAddOfNonZero saw an add of a constant from the same phi and said that its output couldn't be equal. But Basic Alias Analysis was really asking about the value from the previous loop iteration.

This patch at least makes that case not happen anymore, I'm not sure if there were still other ways this can fail. As was discussed in the bug, it looks like fixing BasicAA would be difficult so this patch seemed like a possible workaround

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper created this revision.May 12 2017, 9:32 AM

Can we add a mode (e.g. some extra boolean parameter) to ValueTracking to enable this kind of restriction? We could then enable this mode when in BasicAA. I don't want to, in general, disable things here to work around the fact that BasicAA is doing some unsound.

I couldn't come up with a bool variable name that I liked so I just taught BasicAA not to call isKnownNonEqual if one of the inputs is a PHI. Instead we'll just compute the known bits separately. I think we should add a helper method to KnownBits to check if two KnownBits structs are known non equal which would simplify the intersection check.

craig.topper removed a subscriber: hfinkel.

What is the performance implication of this change-set? Have you done any performance testing? This seems to make the alias analysis very conservative. It is now going to find two non-equal numbers equal very easily.

Only one of the benchmarks I ran this through showed any effect on the compiled code. And the one benchmark that changed got slightly faster.

dberlin edited edge metadata.Jun 14 2017, 10:50 PM

What is the performance implication of this change-set? Have you done any performance testing? This seems to make the alias analysis very conservative. It is now going to find two non-equal numbers equal very easily.

Other than rewrite it all, what are you suggesting he do?

dberlin accepted this revision.Jun 14 2017, 10:50 PM

I'm fine with this.
If we want better performance or better aliasing, we can do the hard work of doing it right.

This revision is now accepted and ready to land.Jun 14 2017, 10:50 PM
This revision was automatically updated to reflect the committed changes.