This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Expand predecessor search to multiple blocks when reverting WhileLoopStarts
ClosedPublic

Authored by dmgreen on May 11 2021, 12:03 PM.

Details

Summary

We were previously only searching a single preheader for call instructions when reverting WhileLoopStarts to DoLoopStarts. This extends that to multiple blocks that can come up when, for example a loop is expanded from a memcpy. It also expends the instructions from just Call's to also include other LoopStarts, to catch other low overhead loops in the preheader.

Diff Detail

Unit TestsFailed

Event Timeline

dmgreen created this revision.May 11 2021, 12:03 PM
dmgreen requested review of this revision.May 11 2021, 12:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2021, 12:03 PM
SjoerdMeijer added inline comments.May 13 2021, 1:27 AM
llvm/lib/Target/ARM/MVETPAndVPTOptimisationsPass.cpp
269

Bikeshedding names: CheckForLRUseInPreheader. I thought the clue was that we not only search the Preheader. So perhaps this should be something like CheckForLRUseInPredecessors?

282

Nit, instead of the big if, an early exit/continue?

if (!IsInvalidTPInstruction(MI))
  continue;
llvm/test/CodeGen/Thumb2/LowOverheadLoops/memcall.ll
279

would it be good to have a MIR test for this too?

dmgreen updated this revision to Diff 345082.May 13 2021, 4:00 AM

Added mir test, renamed to CheckForLRUseInPredecessors and reversed if condition.

SjoerdMeijer accepted this revision.May 13 2021, 5:26 AM

Cheers, LGTM.

This revision is now accepted and ready to land.May 13 2021, 5:26 AM
malharJ added inline comments.May 13 2021, 6:07 AM
llvm/lib/Target/ARM/MVETPAndVPTOptimisationsPass.cpp
269

Since the function name has changed, this (and also in the function declaration) parameter name
could also be changed to "Predecessor" ?

281

Uhm, just trying to understand the original logic ...

but why does a call (in the Preheader) result in reversion of t2WhileLoopStartLR to t2DoLoopStart ?

llvm/test/CodeGen/Thumb2/LowOverheadLoops/wls-search-pred.mir
38–40

do we need llvm.memcpy and llvm.memmove declarations here ?

malharJ added inline comments.May 14 2021, 1:10 AM
llvm/lib/Target/ARM/MVETPAndVPTOptimisationsPass.cpp
281

What I meant was wouldn't a call (in the preheader/predecessor) be placed before the t2WhileLoopStartLR (since it's a terminator) ... and hence it should not matter if LR get's overwritten by the call ?

dmgreen added inline comments.May 14 2021, 6:51 AM
llvm/lib/Target/ARM/MVETPAndVPTOptimisationsPass.cpp
269

No, it's a preheader. Though the naming gets a bit fuzzy. There was a preheader that is split into multiple blocks, which this has to search through. They together will make up the predecessors. And eventually reach the block with the WhileLoopStart (which in other contexts is also called the predecessor.)

281

This is after the WhileLoopStart, but before the actual loop. There can be other instructions/blocks in that preheader before the loop, including calls and whatnot. The block that contains the WhileLoopStart isn't the same as the block that we put the DoLoopStart in.

This revision was landed with ongoing or failed builds.May 14 2021, 7:09 AM
This revision was automatically updated to reflect the committed changes.