This is an archive of the discontinued LLVM Phabricator instance.

[ValueTracking] Use Inst::comesBefore in isValidAssumeForCtx (NFC).
ClosedPublic

Authored by fhahn on Mar 16 2020, 5:52 AM.

Details

Summary

D51664 added Instruction::comesBefore which should provide better
performance than the manual check.

Diff Detail

Unit TestsFailed

Event Timeline

fhahn created this revision.Mar 16 2020, 5:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2020, 5:52 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
nikic added inline comments.Mar 16 2020, 6:21 AM
llvm/lib/Analysis/ValueTracking.cpp
603

This condition is redundant, it's already checked above.

604

This should be if (Inv->comesBefore(CxtI)) return true, otherwise the reverse-direction scan below will be skipped.

fhahn updated this revision to Diff 251109.Mar 18 2020, 9:22 AM
fhahn marked 3 inline comments as done.

Remove unnecessary checks, do not skip the checks later on.

nikic added inline comments.Mar 18 2020, 10:19 AM
llvm/lib/Analysis/ValueTracking.cpp
605

Isn't the !DT condition still relevant here? If we have DT, then this was already covered by the domination check.

Generally, I think this code is a bit convoluted, and I think it would be clearer to rewrite along these lines:

BasicBlock *InvBB = Inv->getParent(), *CxtBB = CxtI->getParent();
if (InvBB != CxtBB) {
  if (DT)
    return DT->dominates(InvBB, CxtBB);
  else
    // We don't have a DT, but this trivially dominates.
    return InvBB == CxtBB->getSinglePredecessor();
}

// If Inv and CtxI are in the same block, check if the assume (Inv) is first
// in the BB.
if (Inv->comesBefore(CxtI))
  return true;

// Rest of the code...
fhahn updated this revision to Diff 254160.Apr 1 2020, 4:07 AM

Generally, I think this code is a bit convoluted, and I think it would be clearer to rewrite along these lines.

Agreed, I've restructured the code a bit. It seemed a bit more straight forward to check and handle the case were both instructions are in the same BB up front, followed by the DT check. WDYT?

nikic accepted this revision.Apr 4 2020, 9:50 AM

LGTM

This revision is now accepted and ready to land.Apr 4 2020, 9:50 AM
This revision was automatically updated to reflect the committed changes.