This is an archive of the discontinued LLVM Phabricator instance.

ARMLoadStoreOptimizer: Rewrite LDM/STM matching logic.
ClosedPublic

Authored by MatzeB on May 29 2015, 2:48 PM.

Details

Summary

This improves the logic in several ways and is a preparation for
followup patches:

  • First perform an analysis and create a list of merge candidates, then transform. This simplifies the code in that you have don't have to care to much anymore that you may be holding iterators to MachineInstrs that get removed.
  • Analyze/Transform basic blocks in reverse order. This allows to use LivePhysRegs to find free registers instead of the RegisterScavenger. The RegisterScavenger will become less precise in the future as it relies on the deprecated kill-flags.
  • Return the newly created node in MergeOps so there's no need to look around in the schedule to find it.
  • Rename some MBBI iterators to InsertBefore to make their role clear.
  • General code cleanup.

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB updated this revision to Diff 26822.May 29 2015, 2:48 PM
MatzeB retitled this revision from to ARMLoadStoreOptimizer: Rewrite LDM/STM matching logic..
MatzeB updated this object.
MatzeB edited the test plan for this revision. (Show Details)
MatzeB added reviewers: rengolin, mroth, jmolloy.
MatzeB set the repository for this revision to rL LLVM.
MatzeB added a subscriber: Unknown Object (MLST).
jmolloy edited edge metadata.Jun 11 2015, 2:27 AM

Hi Mattias,

Sorry it took so long for me to get to this, for some reason Phab's mail didn't go direct to my inbox but got buried in list mail.

I have some comments, some of them are just me being dim, needing some enlightenment :)

Cheers,

James

lib/Target/ARM/ARMLoadStoreOptimizer.cpp
101 ↗(On Diff #26822)

What does the "Idx" refer to? Position from the end of a block?

103 ↗(On Diff #26822)

Again, from the back or front of the block? You've mentioned reverse position in the above struct, so disambiguation would be good here.

514 ↗(On Diff #26822)

Does/should this respect the AllocationOrder for RegClass?

522 ↗(On Diff #26822)

I think it's ambiguous what "Before" means here, given it operates in reverse order. Is this before-when-going-back-to-front?

562 ↗(On Diff #26822)

Any particular reason you upped this limit?

667 ↗(On Diff #26822)

Why has this been removed?

MatzeB updated this revision to Diff 28075.Jun 19 2015, 7:02 PM
MatzeB edited edge metadata.
MatzeB removed rL LLVM as the repository for this revision.

Address review comments.

lib/Target/ARM/ARMLoadStoreOptimizer.cpp
101 ↗(On Diff #26822)

I added a comment:

/// Index in Instrs of the instruction being latest in the schedule.
103 ↗(On Diff #26822)

I added a comment:

/// Index into the basic block where the merged instruction will be
/// inserted. (See MemOpQueueEntry.Position)
514 ↗(On Diff #26822)

I had modeled this after the behaviour of the RegisterScavenger, but you are right taking the allocation order into account is a good thing; I changed it to use the allocation order.

522 ↗(On Diff #26822)

Hmm I always have the problem that you often see code with a "position" pointing at an instruction and a set of registers that are live at that instruction. However that is highly ambiguous since you can only talk about live register immediately before or after an instruction, so when I write code I always use "before" or "after" when talking about liveness at an instruction. Anyway I changed the comment to resolve the ambiguity.

562 ↗(On Diff #26822)

We didn't hit one case in a lit-test anymore and this fixed it, I think this is because we now use the "InsertBefore" instead of the "InsertAfter" position.

667 ↗(On Diff #26822)

This is still happening but as part of MergeOpsUpdate() now around line 831.

MatzeB updated this revision to Diff 28172.Jun 22 2015, 4:37 PM
MatzeB set the repository for this revision to rL LLVM.

New revision which also:

  • Removes an unnecessary "if (NumRegs <= 1) return false;" from MergeOps()
  • change "DebugLoc dl" into "DebugLoc DL"
  • rebased on top of http://reviews.llvm.org/D10620
MatzeB updated this revision to Diff 28424.Jun 24 2015, 4:59 PM

New revision picking up some cleanups in FormCandidates() suggested in the review of http://reviews.llvm.org/D10623

MatzeB updated this revision to Diff 28888.Jul 1 2015, 11:56 AM
MatzeB added a subscriber: t.p.northover.

New revision adapted to LLVM ToT. This changes the way r241003 (+CC Tim) deals with the problem where the base adjustment for an stm had an erroneous kill-flag by pushing the fixes to the code that creates those adjustments, so we don't unnecessarily loose the kill flag on the stm.

rengolin edited edge metadata.Jul 3 2015, 4:54 AM

Hi Matthias,

I'll wait for James' reviews, as this patch is too big and dense and he already had a look at it. In the future, please split the cosmetics (like renaming dl to DL) from feature patches. Each one of the bullets in your description would have been, separately, an easy patch to review.

If James is happy with the patch as is, I won't ask you to split it. But please do next time.

cheers,
--renato

jmolloy accepted this revision.Jul 6 2015, 2:41 AM
jmolloy edited edge metadata.

Hi Matthias,

Sorry for the delay, I was on vacation. I'm happy as long as Tim is.

Cheers,

James

This revision is now accepted and ready to land.Jul 6 2015, 2:41 AM
t.p.northover accepted this revision.Jul 9 2015, 2:14 PM
t.p.northover added a reviewer: t.p.northover.

I didn't spot anything horribly wrong (certainly nothing newly horribly wrong). The only thing that really needs fixing now is the dead code, which you can probably just remove.

lib/Target/ARM/ARMLoadStoreOptimizer.cpp
353 ↗(On Diff #28888)

Maybe isLaodSingle or something? The name seems a bit too generic for what it's actually doing.

413 ↗(On Diff #28888)

Looks dodgy, but I see you just moved this function around.

I've not quite managed to produce an example, but I'd be entirely unsurprised to see something like this used to load a Q-reg:

VLDMDIA %R0, %D0<def-dead>, %D1<def-dead>, %Q0<imp-def>
787 ↗(On Diff #28888)

Commented code. Actually, commented code that I added, I think. It was written to prevent:

add rNewBase, rBase<kill>, #12
stm rNewBase, {..., rBase, ...}

where the Base was killed prematurely if it ended up actually being used in the store. This caused "-verify-machineinstr" failures, but nothing else that I've observed.

It looks like your code aroung line 654 replaces it adequately though.

This revision was automatically updated to reflect the committed changes.