This is an archive of the discontinued LLVM Phabricator instance.

[LoopReroll] Allow for multiple loop control only induction vars
ClosedPublic

Authored by caojoshua on Jan 5 2023, 10:58 PM.

Details

Summary

Before this, LoopReroll would fail an assertion, falsely assuming that
there can only possibly a single loop control only induction variable.

For example:

%a = phi i16 [ %dec2, %for.body ], [ 0, %entry ]
%b = phi i16 [ %dec1, %for.body ], [ 0, %entry ]
%a.next = add nsw i16 %1, -1
%b.next = add nsw i16 %0, -1
%add = add nsw i16 %a, %b
; ... rerollable code
%cmp.not = icmp eq i16 -10, %add
br i1 %cmp.not, label %exit, label %loop

Both %a and %b are valid loop control only induction vars

Additionally, some NFC changes to remove unnecessary isa<PHINode> check

Updated complex_reroll checks

Diff Detail

Event Timeline

caojoshua created this revision.Jan 5 2023, 10:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 5 2023, 10:58 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
caojoshua published this revision for review.Jan 5 2023, 10:59 PM
caojoshua added a reviewer: uabelho.
Herald added a project: Restricted Project. · View Herald TranscriptJan 5 2023, 11:00 PM
uabelho resigned from this revision.Jan 9 2023, 1:11 AM

I've verified that this solves the crash I saw in
https://github.com/llvm/llvm-project/issues/53128
but I don't know this code and have no clue if this is the proper fix or not.

kawashima-fj requested changes to this revision.EditedJan 12 2023, 3:30 AM

@caojoshua Thanks for the fix. Almost looks good to me. I added some minor comments.

llvm/lib/Transforms/Scalar/LoopRerollPass.cpp
199

to is dropped. Is it intentional?

390

It is better to rename the parameter to LoopCtrlIVs (add s).

1195

This if statement may be able to be removed.

  • LoopControlIV is always non-null.
  • IV is one of PossibleIVs. And in the collectPossibleIVs function, a IV never goes to both LoopControlIVs and PossibleIVs.
This revision now requires changes to proceed.Jan 12 2023, 3:30 AM
caojoshua updated this revision to Diff 488673.Jan 12 2023, 8:41 AM
caojoshua marked an inline comment as done.

Thanks for review. Implemented all feedback.

caojoshua marked an inline comment as done.Jan 12 2023, 8:42 AM
caojoshua added inline comments.
llvm/lib/Transforms/Scalar/LoopRerollPass.cpp
199

It was a mistake. Added it back.

kawashima-fj accepted this revision.Jan 12 2023, 9:01 AM

LGTM. Thanks.

This revision is now accepted and ready to land.Jan 12 2023, 9:01 AM
This revision was landed with ongoing or failed builds.Jan 12 2023, 9:01 PM
This revision was automatically updated to reflect the committed changes.