This is an archive of the discontinued LLVM Phabricator instance.

Differential D155718

[InstCombine] Remove unneeded isa<PHINode> check in foldOpIntoPhi
ClosedPublic

Authored by 0xdc03 on Jul 19 2023, 8:42 AM.

Details

Reviewers
nikic
Commits
rGd53b3df570e3: [InstCombine] Remove unneeded isa<PHINode> check in foldOpIntoPhi
Summary

This check is redundant as it is covered by the call to
isPotentiallyReachable.

Depends on D155726.

Diff Detail

Unit TestsFailed

TimeTest
50 msx64 debian > LLVM.Analysis/ValueTracking::known-power-of-two-urem.ll

Event Timeline

0xdc03 created this revision.Jul 19 2023, 8:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2023, 8:42 AM
Herald added subscribers: StephenFan, hiraditya. · View Herald Transcript
0xdc03 requested review of this revision.Jul 19 2023, 8:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2023, 8:42 AM
Herald added a subscriber: llvm-commits. · View Herald Transcript
0xdc03 mentioned this in D155726: [InstCombine] Test case for D155718.Jul 19 2023, 10:20 AM
Harbormaster completed remote builds in B246566: Diff 542048.Jul 19 2023, 10:21 AM
0xdc03 updated this revision to Diff 542099.Jul 19 2023, 10:23 AM
  • Add test case
0xdc03 edited the summary of this revision. (Show Details)Jul 19 2023, 10:24 AM
0xdc03 added a parent revision: D155726: [InstCombine] Test case for D155718.
Harbormaster completed remote builds in B246604: Diff 542099.Jul 19 2023, 1:35 PM
0xdc03 updated this revision to Diff 542297.Jul 19 2023, 9:25 PM
  • Rebase on updated test
  • Fix broken test
Harbormaster completed remote builds in B246757: Diff 542297.Jul 20 2023, 12:02 AM
0xdc03 added a comment.Jul 20 2023, 12:42 AM

Looks like the build timed out on windows...

nikic accepted this revision.Jul 20 2023, 12:49 AM

The change LGTM, but this patch should not be based on D154064. We should land this first and D154064 later.

This revision is now accepted and ready to land.Jul 20 2023, 12:49 AM
0xdc03 updated this revision to Diff 542364.Jul 20 2023, 1:24 AM
  • Invert the patch stack
0xdc03 removed a parent revision: D155726: [InstCombine] Test case for D155718.Jul 20 2023, 1:24 AM
0xdc03 added a child revision: D155726: [InstCombine] Test case for D155718.
In D155718#4517570, @nikic wrote:

The change LGTM, but this patch should not be based on D154064. We should land this first and D154064 later.

Okay, I have done this and changed the patch order but the issue I have now is that no changes are visible in the tests introduced in D155726.

0xdc03 edited the summary of this revision. (Show Details)Jul 20 2023, 1:31 AM
nikic added a comment.Jul 20 2023, 1:37 AM
In D155718#4517738, @0xdc03 wrote:
In D155718#4517570, @nikic wrote:

The change LGTM, but this patch should not be based on D154064. We should land this first and D154064 later.

Okay, I have done this and changed the patch order but the issue I have now is that no changes are visible in the tests introduced in D155726.

The patch order should be D155726, then D155718, then D154064. If you order the patches that way you should see a test change on SwitchTest. It's expected that you don't see a change on BranchTest, which only makes sure that D154064 doesn't regress it.

0xdc03 updated this revision to Diff 542375.Jul 20 2023, 1:49 AM
  • Change the patch order
Harbormaster completed remote builds in B246815: Diff 542375.Jul 20 2023, 1:50 AM
0xdc03 added a child revision: D154064: [InstructionSimplify] Limit threadCmpOverPHI recursion depth to 1.Jul 20 2023, 1:50 AM
0xdc03 removed a child revision: D155726: [InstCombine] Test case for D155718.
0xdc03 added a parent revision: D155726: [InstCombine] Test case for D155718.
0xdc03 edited the summary of this revision. (Show Details)
0xdc03 updated this revision to Diff 550711.Aug 16 2023, 5:51 AM
  • Rebase on main
0xdc03 edited the summary of this revision. (Show Details)Aug 16 2023, 5:53 AM
Harbormaster completed remote builds in B252918: Diff 550711.Aug 16 2023, 7:39 AM
Closed by commit rGd53b3df570e3: [InstCombine] Remove unneeded isa<PHINode> check in foldOpIntoPhi (authored by 0xdc03). · Explain WhyAug 16 2023, 8:40 AM
This revision was automatically updated to reflect the committed changes.
0xdc03 added a commit: rGd53b3df570e3: [InstCombine] Remove unneeded isa<PHINode> check in foldOpIntoPhi.
0xdc03 mentioned this in rGe549d578cc2d: [InstCombine] Test cases for D155718.

Revision Contents

PathSize
llvm/
lib/
Transforms/
InstCombine/
1 line

Diff 542048

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

Show First 20 Lines • Show All 1,291 Lines • ▼ Show 20 Lines for (unsigned i = 0; i != NumPHIValues; ++i) {
// be able to remove it in that case. // be able to remove it in that case.
Value *NewVal = simplifyInstructionWithOperands( Value *NewVal = simplifyInstructionWithOperands(
&I, Ops, SQ.getWithInstruction(InBB->getTerminator())); &I, Ops, SQ.getWithInstruction(InBB->getTerminator()));
if (NewVal && NewVal != PN && !match(NewVal, m_ConstantExpr())) { if (NewVal && NewVal != PN && !match(NewVal, m_ConstantExpr())) {
NewPhiValues.push_back(NewVal); NewPhiValues.push_back(NewVal);
continue; continue;
} }
if (isa<PHINode>(InVal)) return nullptr; // Itself a phi.
if (NonSimplifiedBB) return nullptr; // More than one non-simplified value. if (NonSimplifiedBB) return nullptr; // More than one non-simplified value.
NonSimplifiedBB = InBB; NonSimplifiedBB = InBB;
NonSimplifiedInVal = InVal; NonSimplifiedInVal = InVal;
NewPhiValues.push_back(nullptr); NewPhiValues.push_back(nullptr);
// If the InVal is an invoke at the end of the pred block, then we can't // If the InVal is an invoke at the end of the pred block, then we can't
// insert a computation after it without breaking the edge. // insert a computation after it without breaking the edge.
▲ Show 20 Lines • Show All 3,039 Lines • Show Last 20 Lines