Page MenuHomePhabricator

[LoopLoadElim] Make sure all loops are in simplify form. PR48150
ClosedPublic

Authored by mkazantsev on Nov 16 2020, 2:56 AM.

Details

Summary

LoopLoadElim may end up expanding an AddRec from a loop
which is not the current loop. This loop may not be in simplify
form. We figure it out after the no-return point, so cannot bail
in this case.

AddRec requires simplify form to expand. The only way to ensure
this does not crash is to simplify all loops beforehand.

The issue only exists in new PM. Old PM requests LoopSimplify
required pass and it simplifies all loops before the opt begins.

Diff Detail

Event Timeline

mkazantsev created this revision.Nov 16 2020, 2:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 16 2020, 2:56 AM
mkazantsev requested review of this revision.Nov 16 2020, 2:56 AM
mkazantsev edited the summary of this revision. (Show Details)Nov 16 2020, 2:58 AM
mkazantsev removed a reviewer: d.zobnin.bugzilla.
lebedev.ri added a subscriber: lebedev.ri.

(this is new-pm-specific)

aeubanks added inline comments.Nov 20 2020, 9:33 AM
llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp
627

does every loop need to be simplified? or can the call be moved into the processing of the Worklist below?

mkazantsev added inline comments.Nov 22 2020, 11:40 PM
llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp
627

We might need any loop in simplified form, and the worklist only contains inner loops.

I'm not super familiar with this, do you know why this only affects the new PM? (the code looks fine, I'd just like to understand what's going on better)

llvm/test/Transforms/LoopLoadElim/pr-48150.ll
1–2

is this still necessary?

I'm not super familiar with this, do you know why this only affects the new PM? (the code looks fine, I'd just like to understand what's going on better)

void getAnalysisUsage(AnalysisUsage &AU) const override {
  AU.addRequiredID(LoopSimplifyID);

Because of this, all loops in old PM are simplified before the opt starts.

llvm/test/Transforms/LoopLoadElim/pr-48150.ll
1–2

Nope.

mkazantsev edited the summary of this revision. (Show Details)
mkazantsev marked an inline comment as done.
asbirlea accepted this revision.Nov 25 2020, 8:30 AM
asbirlea added inline comments.
llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp
668

Comments on nullptr args.

This revision is now accepted and ready to land.Nov 25 2020, 8:30 AM
aeubanks accepted this revision.Nov 25 2020, 8:49 AM

Oh I see, thanks

I noticed a new crash with this patch when using the old PM.
I wrote
https://bugs.llvm.org/show_bug.cgi?id=49141
about it.