This is an archive of the discontinued LLVM Phabricator instance.

[LoopReroll] Remove unused LoopReroll pass
Needs ReviewPublic

Authored by nikic on May 16 2023, 8:35 AM.

Details

Summary

This pass was introduced in 2013 and has not been enabled by default (or even been part of the pass pipeline) since that time. As far as I know, there is no active work on it, and no path towards default enablement. There is a long list of miscompiles reported against this pass (most of https://github.com/llvm/llvm-project/issues?q=is%3Aissue+is%3Aopen+loop-reroll), which have remained unfixed for years.

In the interest of not keeping unused code in tree, this removes the pass entirely. Should interest in default enablement for this pass appear, it would need a from-scratch review anyway.

Diff Detail

Event Timeline

nikic created this revision.May 16 2023, 8:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2023, 8:35 AM
nikic requested review of this revision.May 16 2023, 8:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2023, 8:35 AM

My team has been using this pass in production for a while. I think we have a few internal bugfixes that never made it upstream, though. If there's interest, I'll get someone on my team to push those fixes.

Granted, the current state of the pass this might not pass codereview standards even with those fixes...

Can we keep this pass around for some time? I have some tentative plan to extend it to cover some scenarios though not concrete yet.

fhahn added a comment.May 17 2023, 8:34 AM

My team has been using this pass in production for a while. I think we have a few internal bugfixes that never made it upstream, though. If there's interest, I'll get someone on my team to push those fixes.

Granted, the current state of the pass this might not pass codereview standards even with those fixes...

I think if there's a concrete plan to fix the open issues at least, then we could keep it. But otherwise the code can be always be brought back .

nikic added a comment.May 22 2023, 2:13 AM

My team has been using this pass in production for a while. I think we have a few internal bugfixes that never made it upstream, though. If there's interest, I'll get someone on my team to push those fixes.

Granted, the current state of the pass this might not pass codereview standards even with those fixes...

I think if there's a concrete plan to fix the open issues at least, then we could keep it. But otherwise the code can be always be brought back .

Yeah, this is the main bit I care about. To keep the pass upstream, there should also be an upstream owner for it, who at least makes sure the correctness issues get fixed. If fixes only get applied downstream, then it's probably better for the whole pass to live downstream.

fhahn added a comment.May 22 2023, 8:50 AM

Apologies, I missed that @nikic already included a GitHub issue search

dewen added a subscriber: dewen.Aug 10 2023, 8:01 PM

My team has been using this pass in production for a while. I think we have a few internal bugfixes that never made it upstream, though. If there's interest, I'll get someone on my team to push those fixes.

Granted, the current state of the pass this might not pass codereview standards even with those fixes...

Hello. May I ask if your colleagues here will push the relevant fixes upstream in the future?

Hello, is there a clear schedule for Loop Reroll pass to fix? @efriedma

fhahn added a comment.Wed, Nov 22, 1:36 AM

Ping, any update on this? I think we should remove the pass temporarily unless someone has time to address the known issues soon-ish.

bjope added a subscriber: bjope.Wed, Nov 22, 2:23 AM
xgupta added a subscriber: xgupta.Wed, Nov 22, 4:04 AM

This pass is actually not working at all with Clang as it is not moved to the new pass manager - https://github.com/llvm/llvm-project/issues/59065.

Hi,

Just pointing out that the loop reroller is a documented feature of the clang driver:
https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-freroll-loops

But yes, that command-line option has been broken since the new pass manager (clang-13).

Bruno

llvm/test/Transforms/LoopReroll/negative.ll