This is an archive of the discontinued LLVM Phabricator instance.

[LoopFlatten] Widen the IV
ClosedPublic

Authored by SjoerdMeijer on Nov 2 2020, 12:52 PM.

Details

Summary

Widen the IV to the widest available and legal integer type, which makes this transformations always safe so that we can skip overflow checks.

Motivation is to let this pass trigger on 64-bit targets too, and this is the last patch in a serie to achieve this: D90402 moves pass LoopFlatten to just before IndVarSimplify so that IVs are not already widened, D90421 factors out widening from IndVarSimplify into LoopUtils so that we can also use it in LoopFlatten, and D90408 is a refactoring in LoopFlatten to make it easier to reuse and rediscover loop components again after widening.

Diff Detail

Event Timeline

SjoerdMeijer created this revision.Nov 2 2020, 12:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 2 2020, 12:52 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
SjoerdMeijer requested review of this revision.Nov 2 2020, 12:52 PM
dmgreen added inline comments.Nov 9 2020, 1:17 AM
llvm/lib/Transforms/Scalar/LoopFlatten.cpp
339

Some formatting is off here.

351

I think this should be checking each use of the trunk, or checking the trunk has one use.

400

auto *V =

591

>= ?

I feel like something here needs to be checking that newbitwidth > InnerType->getScalarSizeInBits() * 2
(Which likely won't be very important for power-2 type sizes, but is still needed to make sure the new IV doesn't overflow?)

594

Can this call findLoopComponents directly, or is there something in CanFlattenloopPair that would be needed?

604

Do we need to be widening all the phi's, or just the induction?

611

What does DeadInstr contain after the calls to WidenIV? Should they be removed now, does that simplify things later on?

llvm/test/Transforms/LoopFlatten/widen-iv.ll
91

Can this test be cleaned up a bit (or perhaps a simpler test be added). This seems to be loop unswitched with this extra remainder loop left over.

Thanks for reviewing!
This is a rebase, will now start addressing the comments.

SjoerdMeijer added inline comments.Nov 10 2020, 3:54 AM
llvm/lib/Transforms/Scalar/LoopFlatten.cpp
591

re: >= ?

I didn't see how the inner type can be larger than the legal max size, but since > certainly doesn't hurt I will add that.

594

Yeah, I looked at that, but there's a bit more going on there, e.g. it checks and sets the IV users. So, it's best and easiest just to run CanFlattenLoopPair again.

604

I borrowed this logic that drives createWideIV from IndVarSimplify, but that might indeed be interested in promoting all of them. But I guess you're right that we are interested in just the induction phi, so will look into that.

611

It's empty, because they are not "trivially" dead. I had a go at trying to clean it up, but it wasn't trivial, and needed a lot of code, for which we have other passes. You'll see some of the old phis in test cases, but they will cleaned up later by other passes.

Comments addressed

SjoerdMeijer added inline comments.Nov 10 2020, 11:34 AM
llvm/test/Transforms/LoopFlatten/widen-iv.ll
91

I have kept the test because this is my motivating example, and the IR input to loop flatten when it is generated from source. Skipping loop unswitch caused this to no longer trigger, and the test isn't that big, so I would prefer to keep it. If you insist, I will have another look though.

dmgreen added inline comments.Nov 11 2020, 2:27 PM
llvm/lib/Transforms/Scalar/LoopFlatten.cpp
353

Why continue, not returning false?

591

As far as I understand, "legal" here just means "can fit into a single register", so 64bits on aarch64. There may already be IV's that are larger.

611

Ah OK. Is it possible to call DeleteDeadPhi's on the loops? It might simplify some of the "OrigPHIs" logic if they have already been removed.

llvm/test/Transforms/LoopFlatten/widen-iv.ll
91

Yeah, I was mostly meaning to remove the blocks like for.cond1.preheader and for.cond1.preheader.preheader which is not part of the flattened loop. I think it should keep working on what remains if you manually edit it. If not and it doesn't keep working, but a big deal.

Comments addressed:

  • now using RecursivelyDeleteDeadPHINode at the end to clean up dead phis. Doing this earlier proved to be a bit tricky with a few things in flight, but at least a nice clean up at the end.
  • thanks for the suggestion: the test case has now been cleaned up.

now using RecursivelyDeleteDeadPHINode at the end to clean up dead phis. Doing this earlier proved to be a bit tricky with a few things in flight, but at least a nice clean up at the end.

What goes wrong when these are done earlier? I was hoping that it would help remove the need for OrigPHIs and Wide2OrigPHIs, cleaning up a bit of the awkwardness they are needed for.

llvm/test/Transforms/LoopFlatten/widen-iv.ll
7

dso_local and local_unnamed_addr #0 can be removed.

Was doing the RecursiveDelete in the wrong place. With that fixed we indeed no longer need the extra bookkeeping of OrigPhis and Orig2Wide, so that has been cleaned up.

dmgreen accepted this revision.Nov 13 2020, 6:58 AM

Thanks. LGTM.

This revision is now accepted and ready to land.Nov 13 2020, 6:58 AM
This revision was automatically updated to reflect the committed changes.