Page MenuHomePhabricator

[LoopFlatten] Fix invalid assertion (PR49571)
ClosedPublic

Authored by TaWeiTu on Wed, Mar 24, 2:55 AM.

Details

Summary

The InductionPHI is not necessarily the increment instruction, as
demonstrated in pr49571.ll.
This patch removes the assertion and instead bails out from the
LoopFlatten pass if that happens.

This fixes https://bugs.llvm.org/show_bug.cgi?id=49571

Diff Detail

Event Timeline

TaWeiTu created this revision.Wed, Mar 24, 2:55 AM
TaWeiTu requested review of this revision.Wed, Mar 24, 2:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Mar 24, 2:55 AM
SjoerdMeijer accepted this revision.Wed, Mar 24, 3:05 AM

Yep, agreed, seems reasonable. Thanks for fixing.

This revision is now accepted and ready to land.Wed, Mar 24, 3:05 AM

Yep, agreed, seems reasonable. Thanks for fixing.

Thanks!

This revision was landed with ongoing or failed builds.Wed, Mar 24, 3:08 AM
This revision was automatically updated to reflect the committed changes.
fhahn added inline comments.Wed, Mar 24, 3:09 AM
llvm/lib/Transforms/Scalar/LoopFlatten.cpp
181

The message seems a bit confusing, perhaps it should say Incoming value from latch is not the increment inst?

llvm/test/Transforms/LoopFlatten/pr49571.ll
4

given that the test is very simple, I think it would be better to explicitly check the full output. Also, it would be helpful if you could add a comment explaining what scenario this tests

Hi Florian,

Sorry for the haste commit.
I will fix these issues in a NFC patch shortly.
Is that OK or should I revert this patch for now?

SjoerdMeijer added inline comments.Wed, Mar 24, 3:12 AM
llvm/test/Transforms/LoopFlatten/pr49571.ll
4

Ah yeah, agreed, I missed that. A follow up commit should do, if you're comfortable I don't think that needs another review, but I guess we are happy to look again of course. Definitely not needed to revert the first commit.

The debug message and test description are improved in rG4d9d7368759