This is an archive of the discontinued LLVM Phabricator instance.

Differential D52085

[ARM][ARMLoadStoreOptimizer]
ClosedPublic

Authored by LukeCheeseman on Sep 14 2018, 3:48 AM.

Details

Reviewers
javed.absar
efriedma
MatzeB
Commits
rGbda54bca391d: [ARM][ARMLoadStoreOptimizer]
rL342872: [ARM][ARMLoadStoreOptimizer]
Summary
  • The load store optimizer is currently merging multiple loads/stores into VLDM/VSTM with more than 16 doubleword registers
  • This is an UNPREDICTABLE instruction and shouldn't be done
  • It looks like the Limit for how many registers included in a merge got dropped at some point so I am reintroducing it in this patch
  • This fixes https://bugs.llvm.org/show_bug.cgi?id=38389

Diff Detail

Event Timeline

LukeCheeseman created this revision.Sep 14 2018, 3:48 AM
Herald added a reviewer: javed.absar. · View Herald TranscriptSep 14 2018, 3:48 AM
Herald added subscribers: llvm-commits, chrib, kristof.beyls, srhines. · View Herald Transcript
LukeCheeseman edited the summary of this revision. (Show Details)Sep 14 2018, 7:41 AM
efriedma added reviewers: efriedma, MatzeB.Sep 17 2018, 2:32 PM
efriedma added a subscriber: efriedma.

How hard would it be to generate two vstmia instructions? I think you can just move the limit check into the "Merge following instructions where possible" loop. The generated code is more efficient, and it's easier to verify the testcase doesn't break for some other reason.

lib/Target/ARM/ARMLoadStoreOptimizer.cpp
1094

This limit isn't actually useful...? There are only 32 float registers anyway.

1859

How is this change related?

LukeCheeseman updated this revision to Diff 165988.Sep 18 2018, 9:02 AM
  • Remove extra limit cases that were unnecessary
  • Check the Count/Limit on each iteration of the merge loop
  • Replace guard with assert, previously only candidates that couldn't be merge were one instruction long
efriedma added inline comments.Sep 18 2018, 2:29 PM
lib/Target/ARM/ARMLoadStoreOptimizer.cpp
1054

Instead of disabling merging, could we just do something like "if (Count == Limit) break;", so we merge the first 16 stores, then consider any remaining stores as a separate set?

MatzeB added a comment.Sep 18 2018, 4:46 PM

Please create a .mir test that just checks the ARMLoadStoreOptimizer pass. See test/CodeGen/ARM/vldm-liveness.mir or test/CodeGen/ARM/load_store_opt_kill.mir for example.

LukeCheeseman added inline comments.Sep 19 2018, 3:40 AM
lib/Target/ARM/ARMLoadStoreOptimizer.cpp
1054

Ah yes, that's a much better idea. Thanks

LukeCheeseman updated this revision to Diff 166116.Sep 19 2018, 6:31 AM
  • Break out of the merge loop so that 16 doubleword register loads get merged and the next registers get inspected independently
  • Add a MIR test that checks only the arm load store optimizer pass
LukeCheeseman marked 2 inline comments as done.Sep 19 2018, 6:32 AM
LukeCheeseman updated this revision to Diff 166305.Sep 20 2018, 8:48 AM
  • Removing some extraneous MIR input
efriedma added a comment.Sep 20 2018, 12:53 PM

MIR can be simplified more; see http://llvm.org/docs/MIRLangRef.html#simplifying-mir-files .

LukeCheeseman updated this revision to Diff 166430.Sep 21 2018, 3:20 AM
  • Reduce MIR test, remove lots of unnecessary info
LukeCheeseman updated this revision to Diff 166432.Sep 21 2018, 3:25 AM
LukeCheeseman updated this revision to Diff 166434.Sep 21 2018, 3:30 AM
MatzeB accepted this revision.Sep 21 2018, 9:12 AM

The new .mir test is nicely concise now. Maybe we can even do without the .ll test now?

LGTM with nitpicks addressed and when Eli agrees.

test/CodeGen/ARM/load_store_opt_reg_limit.mir
3–11

You should be able to drop the whole IR block too I think.

13

Better style: # CHECK-LABEL: name: _Z2brR1lI1ME (may also consider choosing a simpler function name)

This revision is now accepted and ready to land.Sep 21 2018, 9:12 AM
LukeCheeseman updated this revision to Diff 166498.Sep 21 2018, 10:16 AM
  • Remove .ll test and simplify mir test
LukeCheeseman marked 2 inline comments as done.Sep 21 2018, 10:16 AM
efriedma accepted this revision.Sep 21 2018, 11:07 AM

LGTM

LukeCheeseman closed this revision.Sep 24 2018, 5:29 AM

Closed by r342872

Revision Contents

PathSize
lib/
Target/
ARM/
14 lines
test/
CodeGen/
ARM/
40 lines

Diff 166498

lib/Target/ARM/ARMLoadStoreOptimizer.cpp

Show First 20 Lines • Show All 1,021 Lines • ▼ Show 20 Lines do {
// deprecated; LDM to PC is fine but cannot happen here. // deprecated; LDM to PC is fine but cannot happen here.
if (PReg == ARM::SP || PReg == ARM::PC) if (PReg == ARM::SP || PReg == ARM::PC)
CanMergeToLSMulti = CanMergeToLSDouble = false; CanMergeToLSMulti = CanMergeToLSDouble = false;
// Should we be conservative? // Should we be conservative?
if (AssumeMisalignedLoadStores && !mayCombineMisaligned(*STI, *MI)) if (AssumeMisalignedLoadStores && !mayCombineMisaligned(*STI, *MI))
CanMergeToLSMulti = CanMergeToLSDouble = false; CanMergeToLSMulti = CanMergeToLSDouble = false;
// vldm / vstm limit are 32 for S variants, 16 for D variants.
unsigned Limit;
switch (Opcode) {
default:
Limit = UINT_MAX;
break;
case ARM::VLDRD:
case ARM::VSTRD:
Limit = 16;
break;
}
// Merge following instructions where possible. // Merge following instructions where possible.
for (unsigned I = SIndex+1; I < EIndex; ++I, ++Count) { for (unsigned I = SIndex+1; I < EIndex; ++I, ++Count) {
int NewOffset = MemOps[I].Offset; int NewOffset = MemOps[I].Offset;
if (NewOffset != Offset + (int)Size) if (NewOffset != Offset + (int)Size)
break; break;
const MachineOperand &MO = getLoadStoreRegOp(*MemOps[I].MI); const MachineOperand &MO = getLoadStoreRegOp(*MemOps[I].MI);
unsigned Reg = MO.getReg(); unsigned Reg = MO.getReg();
if (Reg == ARM::SP || Reg == ARM::PC) if (Reg == ARM::SP || Reg == ARM::PC)
break; break;
if (Count == Limit)
break;
// See if the current load/store may be part of a multi load/store. // See if the current load/store may be part of a multi load/store.
efriedmaUnsubmitted
Done
ReplyInline Actions

Instead of disabling merging, could we just do something like "if (Count == Limit) break;", so we merge the first 16 stores, then consider any remaining stores as a separate set?

efriedma: Instead of disabling merging, could we just do something like "if (Count == Limit) break;", so…
LukeCheesemanAuthorUnsubmitted
Not Done
ReplyInline Actions

Ah yes, that's a much better idea. Thanks

LukeCheeseman: Ah yes, that's a much better idea. Thanks
unsigned RegNum = MO.isUndef() ? std::numeric_limits<unsigned>::max() unsigned RegNum = MO.isUndef() ? std::numeric_limits<unsigned>::max()
: TRI->getEncodingValue(Reg); : TRI->getEncodingValue(Reg);
bool PartOfLSMulti = CanMergeToLSMulti; bool PartOfLSMulti = CanMergeToLSMulti;
if (PartOfLSMulti) { if (PartOfLSMulti) {
// Register numbers must be in ascending order. // Register numbers must be in ascending order.
if (RegNum <= PRegNum) if (RegNum <= PRegNum)
PartOfLSMulti = false; PartOfLSMulti = false;
// For VFP / NEON load/store multiples, the registers must be // For VFP / NEON load/store multiples, the registers must be
Show All 23 Lines do {
// Form a candidate from the Ops collected so far. // Form a candidate from the Ops collected so far.
MergeCandidate *Candidate = new(Allocator.Allocate()) MergeCandidate; MergeCandidate *Candidate = new(Allocator.Allocate()) MergeCandidate;
for (unsigned C = SIndex, CE = SIndex + Count; C < CE; ++C) for (unsigned C = SIndex, CE = SIndex + Count; C < CE; ++C)
Candidate->Instrs.push_back(MemOps[C].MI); Candidate->Instrs.push_back(MemOps[C].MI);
Candidate->LatestMIIdx = Latest - SIndex; Candidate->LatestMIIdx = Latest - SIndex;
Candidate->EarliestMIIdx = Earliest - SIndex; Candidate->EarliestMIIdx = Earliest - SIndex;
Candidate->InsertPos = MemOps[Latest].Position; Candidate->InsertPos = MemOps[Latest].Position;
if (Count == 1) if (Count == 1)
efriedmaUnsubmitted
Done
ReplyInline Actions

This limit isn't actually useful...? There are only 32 float registers anyway.

efriedma: This limit isn't actually useful...? There are only 32 float registers anyway.
CanMergeToLSMulti = CanMergeToLSDouble = false; CanMergeToLSMulti = CanMergeToLSDouble = false;
Candidate->CanMergeToLSMulti = CanMergeToLSMulti; Candidate->CanMergeToLSMulti = CanMergeToLSMulti;
Candidate->CanMergeToLSDouble = CanMergeToLSDouble; Candidate->CanMergeToLSDouble = CanMergeToLSDouble;
Candidates.push_back(Candidate); Candidates.push_back(Candidate);
// Continue after the chain. // Continue after the chain.
SIndex += Count; SIndex += Count;
} while (SIndex < EIndex); } while (SIndex < EIndex);
} }
▲ Show 20 Lines • Show All 762 Lines • ▼ Show 20 Lines if (Candidate->CanMergeToLSMulti || Candidate->CanMergeToLSDouble) {
MergeBaseUpdateLSMultiple(Merged); MergeBaseUpdateLSMultiple(Merged);
} else { } else {
for (MachineInstr *MI : Candidate->Instrs) { for (MachineInstr *MI : Candidate->Instrs) {
if (MergeBaseUpdateLoadStore(MI)) if (MergeBaseUpdateLoadStore(MI))
Changed = true; Changed = true;
} }
} }
} else { } else {
assert(Candidate->Instrs.size() == 1); assert(Candidate->Instrs.size() == 1);
efriedmaUnsubmitted
Not Done
ReplyInline Actions

How is this change related?

efriedma: How is this change related?
if (MergeBaseUpdateLoadStore(Candidate->Instrs.front())) if (MergeBaseUpdateLoadStore(Candidate->Instrs.front()))
Changed = true; Changed = true;
} }
} }
Candidates.clear(); Candidates.clear();
// Try to fold add/sub into the LDRD/STRD formed by ARMPreAllocLoadStoreOpt. // Try to fold add/sub into the LDRD/STRD formed by ARMPreAllocLoadStoreOpt.
for (MachineInstr *MI : MergeBaseCandidates) for (MachineInstr *MI : MergeBaseCandidates)
MergeBaseUpdateLSDouble(*MI); MergeBaseUpdateLSDouble(*MI);
▲ Show 20 Lines • Show All 580 Lines • Show Last 20 Lines

test/CodeGen/ARM/load_store_opt_reg_limit.mir

  • This file was added.
# RUN: llc -mtriple=thumbv7--linux-android -verify-machineinstrs -run-pass=arm-ldst-opt %s -o - | FileCheck %s --check-prefix=CHECK-MERGE
#CHECK-MERGE: foo
name: foo
# CHECK-MERGE: VSTMDIA $r4, 14, $noreg, $d15, $d16, $d17, $d18, $d19, $d20, $d21, $d22, $d23, $d24, $d25, $d26, $d27, $d28, $d29, $d30
# CHECK-MERGE-NEXT: VSTRD $d31, $r4, 32, 14, $noreg :: (store 8)
# CHECK-MERGE: VSTMDIA killed $r0, 14, $noreg, $d4, $d5, $d6, $d7, $d8, $d9, $d10, $d11, $d12, $d13, $d14
body: |
bb.0:
VSTRD $d15, $r4, 0, 14, $noreg :: (store 8)
VSTRD $d16, $r4, 2, 14, $noreg :: (store 8)
VSTRD $d17, $r4, 4, 14, $noreg :: (store 8)
MatzeBUnsubmitted
Done
ReplyInline Actions

You should be able to drop the whole IR block too I think.

MatzeB: You should be able to drop the whole IR block too I think.
VSTRD $d18, $r4, 6, 14, $noreg :: (store 8)
VSTRD $d19, $r4, 8, 14, $noreg :: (store 8)
MatzeBUnsubmitted
Done
ReplyInline Actions

Better style: # CHECK-LABEL: name: _Z2brR1lI1ME (may also consider choosing a simpler function name)

MatzeB: Better style: `# CHECK-LABEL: name: _Z2brR1lI1ME` (may also consider choosing a simpler…
VSTRD $d20, $r4, 10, 14, $noreg :: (store 8)
VSTRD $d21, $r4, 12, 14, $noreg :: (store 8)
VSTRD $d22, $r4, 14, 14, $noreg :: (store 8)
VSTRD $d23, $r4, 16, 14, $noreg :: (store 8)
VSTRD $d24, $r4, 18, 14, $noreg :: (store 8)
VSTRD $d25, $r4, 20, 14, $noreg :: (store 8)
VSTRD $d26, $r4, 22, 14, $noreg :: (store 8)
VSTRD $d27, $r4, 24, 14, $noreg :: (store 8)
VSTRD $d28, $r4, 26, 14, $noreg :: (store 8)
VSTRD $d29, $r4, 28, 14, $noreg :: (store 8)
VSTRD $d30, $r4, 30, 14, $noreg :: (store 8)
VSTRD $d31, $r4, 32, 14, $noreg :: (store 8)
VSTRD $d0, $r4, 34, 14, $noreg :: (store 8)
VSTRD $d1, $r4, 36, 14, $noreg :: (store 8)
VSTRD $d3, $r4, 38, 14, $noreg :: (store 8)
VSTRD $d2, $r4, 40, 14, $noreg :: (store 8)
VSTRD $d4, $r4, 42, 14, $noreg :: (store 8)
VSTRD $d5, $r4, 44, 14, $noreg :: (store 8)
VSTRD $d6, $r4, 46, 14, $noreg :: (store 8)
VSTRD $d7, $r4, 48, 14, $noreg :: (store 8)
VSTRD $d8, $r4, 50, 14, $noreg :: (store 8)
VSTRD $d9, $r4, 52, 14, $noreg :: (store 8)
VSTRD $d10, $r4, 54, 14, $noreg :: (store 8)
VSTRD $d11, $r4, 56, 14, $noreg :: (store 8)
VSTRD $d12, $r4, 58, 14, $noreg :: (store 8)
VSTRD $d13, $r4, 60, 14, $noreg :: (store 8)
VSTRD $d14, $r4, 62, 14, $noreg :: (store 8)