Page MenuHomePhabricator

[IRCE] Make IRCE a Function pass.
ClosedPublic

Authored by asbirlea on Jan 28 2020, 4:19 PM.

Details

Summary

Make InductiveRangeCheckElimination a FunctionPass.

Diff Detail

Event Timeline

asbirlea created this revision.Jan 28 2020, 4:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 28 2020, 4:20 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Two drive-by comments only.

llvm/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
1778

I know this is not your code but I fail to see why the LPMAddNewLoop is different here from below. If it wasn't we could merge almost all of the run code into a common helper.

llvm/lib/Transforms/Utils/LoopUtils.cpp
1492

Nit: Loops is not the right name anymore.
We could probably also just call the array ref version with {&L}

mkazantsev added inline comments.Jan 30 2020, 2:07 AM
llvm/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
1764

Why not PreserveLCSSA = true?

1766

Should only be called if simplifyLoop did something. My suggestion is

if (simplifyLoop ...) {
  formLCSSA (if that cannot be achieved by setting flag)
  Changed = true;
}
1806

Same comments as above.

mkazantsev added inline comments.Jan 30 2020, 4:26 AM
llvm/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
1766

Actually, I am not even sure that a function pass should commit to preserve LCSSA. It is something that only loop passes use.

mkazantsev added inline comments.Jan 30 2020, 4:31 AM
llvm/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
260

If we do preserve it, then AU.addPreserved<LCSSAVerificationPass>();?

What was the motivation to make it a function pass? Conceptually it's a loop pass.

Hi Max, please see https://groups.google.com/forum/#!topic/llvm-dev/pOMjqRYZRQ0. I didn't have an up-to-date email address, so the emails cc-ed to you bounced.

My main motivation is that we need BFI computed from scratch, as it's not being preserved. The new pass manager will disallow getting a cached analysis that can be invalidated, in order to avoid hidden bugs (happy to elaborate here). This patch is rebased on top of D72891, which removes the getCachedResult for BFI.

I don't have the context for how IRCE is used, on its own or along with other loop passes, that's why I sent out the RFC, hoping to get some clarifications. Please also see the thread of making LoopUnrollAndJam a Function pass.

Hi Alina,

Thanks for clarification. We currently use IRCE in a loop pipeline along with LoopSimplifyCFG only. I think the only motivation for this was an attempt to save some compile time. Looks like it won't harm if we run them in different pipelines, I don't think there would be difference.

Fine by me.

asbirlea updated this revision to Diff 242179.Feb 3 2020, 1:55 PM
asbirlea marked 8 inline comments as done.

Address comments.

llvm/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
1766

I don't think, as a Function pass we need to promise preserving LCSSA. In the LoopPassManager (if this was a Loop pass), the two passes (simplify and lcssa) are run to canonicalize the loop. This is the only purpose of running these passes here.
The same approach is used by other function passes that aim to do loop optimizations.

1778

Agreed.
@mkazantsev: Does it makes send to update both LPMAddNewLoop to have the if (!IsSubloop) condition (see current diff), or is there a specific reason why they were different?
I can send an NFC merging the two code path after that.

llvm/lib/Transforms/Utils/LoopUtils.cpp
1492

The ArrayRef version above requires an ArrayRef&, so I'd need to allocate a container at the callsite.
Is there another option I missed?

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

asbirlea updated this revision to Diff 242184.Feb 3 2020, 2:12 PM

Rebase on ToT.

Unit tests: pass. 62428 tests passed, 0 failed and 845 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 2 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

asbirlea updated this revision to Diff 242372.Feb 4 2020, 10:34 AM

clang-format.

asbirlea updated this revision to Diff 242374.Feb 4 2020, 10:39 AM

clang-tidy

Unit tests: unknown.

clang-tidy: fail. clang-tidy found 0 errors and 2 warnings. 0 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Unit tests: unknown.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

mkazantsev accepted this revision.Feb 4 2020, 6:52 PM
This revision is now accepted and ready to land.Feb 4 2020, 6:52 PM
This revision was automatically updated to reflect the committed changes.