Page MenuHomePhabricator

[ValueTracking] Allow context-sensitive nullness check for non-pointers
ClosedPublic

Authored by jdoerfert on Tue, Oct 29, 9:51 AM.

Details

Summary
Same as D60846 but with a fix for the problem encountered there which
was a missing context adjustment in the handling of PHI nodes.

Diff Detail

Event Timeline

jdoerfert created this revision.Tue, Oct 29, 9:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Oct 29, 9:51 AM
jdoerfert updated this revision to Diff 226989.Tue, Oct 29, 4:10 PM

Add tests from D60846 and fix the bug exposed there

jdoerfert retitled this revision from [ValueTracking][WIP] Allow context-sensitive nullness check for non-pointers to [ValueTracking] Allow context-sensitive nullness check for non-pointers.
jdoerfert edited the summary of this revision. (Show Details)
nikic added inline comments.Wed, Oct 30, 1:19 AM
llvm/lib/Analysis/InstructionSimplify.cpp
556

You can use Q.getWithInstruction(InTI) here.

llvm/lib/Analysis/ValueTracking.cpp
1906

Why can't this be an assert? Looks like the Constant case is fully handled first in isKnownNonZero().

jdoerfert marked 2 inline comments as done.Wed, Oct 30, 10:11 AM
jdoerfert added inline comments.
llvm/lib/Analysis/InstructionSimplify.cpp
556

Thx, will do.

llvm/lib/Analysis/ValueTracking.cpp
1906

I thought so too, run it, and caused the assert to trigger. The last case has two ifs nested. That is already a way out of the first case, might be more.

I don't think we want to look at dominance of constants (for now) so I just return false. There is actually something we could do here, but hat is a different story.

jdoerfert updated this revision to Diff 227195.Wed, Oct 30, 3:48 PM
jdoerfert edited the summary of this revision. (Show Details)

Simplified as suggested

jdoerfert marked 2 inline comments as done.Wed, Oct 30, 3:49 PM

Any more comments :)

lebedev.ri accepted this revision.Wed, Oct 30, 11:08 PM

Since the regression that reverted D60846 appears to be resolved, LG.

llvm/lib/Analysis/ValueTracking.cpp
1906

While there, should this be

if (isa<Constant>(V))
  return isa<ConstantPointerNull>(V);

?

llvm/test/Transforms/InstCombine/known-non-zero.ll
95

Precommit

This revision is now accepted and ready to land.Wed, Oct 30, 11:08 PM
jdoerfert marked an inline comment as done.Thu, Oct 31, 8:52 AM
jdoerfert added inline comments.
llvm/lib/Analysis/ValueTracking.cpp
1906

I don't think so, and unfortunately !isa<ConstantPointerNull> doesn't work either (but that would be closer). I was hoping that "easy" constants don't make it here and hard ones would need us to inspect the surrounding (and are unlikely anyway, e.g., globals with weak linkage)

This revision was automatically updated to reflect the committed changes.