Page MenuHomePhabricator

[LSR] PR41445: Rewrite misses some fixup locations if it splits critical edge.
ClosedPublic

Authored by dendibakh on Apr 12 2019, 9:56 PM.

Details

Summary

If LSR split critical edge during rewriting phi operands and
phi node has other pending fixup operands, we need to
update those pending fixups. Otherwise formulae will not be
implemented completely and some instructions will not be eliminated.

Diff Detail

Repository
rL LLVM

Event Timeline

dendibakh created this revision.Apr 12 2019, 9:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2019, 9:56 PM

The source change looks good to me, but I would like a bit more work on the test itself.

Comments below.

lib/Transforms/Scalar/LoopStrengthReduce.cpp
5359 ↗(On Diff #195000)

We don't usually put PR number in comments. Just make sure to put it in your commit message.

5381 ↗(On Diff #195000)

To reduce indentation, use:

if (foundOriginalPHI)
  continue;
test/Transforms/LoopStrengthReduce/pr41445.ll
1 ↗(On Diff #195000)

Use a more descriptive filename please: e.g., missing-phi-operand-update.

1 ↗(On Diff #195000)

Please run opt -instnamer to get rid of the implicit variable.

2 ↗(On Diff #195000)

Put the PR number in comments in the test.

5 ↗(On Diff #195000)

Add a comment describing the characteristic of the test.

6 ↗(On Diff #195000)

Could you add a test that fall into foundInOriginalPHI == true if that's not already the case?

dendibakh marked 7 inline comments as done.

Fixed comments.

test/Transforms/LoopStrengthReduce/pr41445.ll
6 ↗(On Diff #195000)

Yes, all those three cases

  • found in original PHI
  • found in incoming basic block PHIs
  • not found anywhere

happen in this scenario.

qcolombet accepted this revision.Apr 15 2019, 2:23 PM

Looks good!

This revision is now accepted and ready to land.Apr 15 2019, 2:23 PM

@qcolombet , can you please commit this patch on my behalf?

This revision was automatically updated to reflect the committed changes.