[AArch64] Query the target when folding loads and stores

[evandro]

Summary

Consider the processor cost model when folding loads and stores to use the pre or post index addressing modes.

[fhahn]

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

591

This is just moving some code from optimizeBlock? If that's unrelated having this in a separate patch would make it slightly easier to review IMO :)

1674

This is just moving some code from optimizeBlock?

[mcrosier]

The load/store opt pass is already pretty expensive in terms of compile-time. Did you see any compile-time regressions in your testing? Also, what performance results have you collected?

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

592

+1 to Florian's comment. Also, this comment should remain in optimizeBlock.

1408

Should be

} else {

[mcrosier]

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

653

You could just add SM and STI to the AArch64LoadStoreOpt class and then initialize it in runOnMachineFunction.

682

You can drop this else.

1380

Add a comment:

// Evaluate if the new instruction is a better choice than the old ones.

1381

I think you can return NextI here. This will ensure we skip the Update instruction (i.e., the base address add/sub), which is never a candidate for LdStUpdateMerging.

[SirishP]

In D39976#924618, @mcrosier wrote:

The load/store opt pass is already pretty expensive in terms of compile-time. Did you see any compile-time regressions in your testing? Also, what performance results have you collected?

Cost model check is done at the very end when we are merging instructions, after most of the other checks are already done. Having said that, yes compile time regression test is a good idea. As for the performance, A72 and A57 both have higher latency with decoder bubble on load with pre-post update. That is definitely going to benefit where we look at cost model and decide not to create LD update. If we were to leave LD/ADD in epilogue and this pass not create LD update instruction, that's benefit as well.

[junbuml]

No objection in high level. IMHO, it will not cause significant compile time regression, but better to make sure. I also agree with splitting this patch: one for the profitability check and one for refactoring.

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

649

No need to repeat this check for every candiate.

667–669

Can we guarantee these pointers are always none null?

[evandro]

There's some code refactoring here that I'll put in a separate patch soon.

Also, since the the extra check is only performed for candidate instructions, the impact on compile time is negligible.

The performance difference can be significant, especially in FP benchmarks with tight loops over gobbles of data.

Thank you for your feedback.

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

667–669

Yes because getSchedClassDesc() returns the address of a static array element.

[evandro]

Please, let me know if I addressed your concerns before I split the code refactoring in another patch.

Thank you,

[mcrosier]

In D39976#925078, @evandro wrote:

Please, let me know if I addressed your concerns before I split the code refactoring in another patch.

One minor comment, but feel free to go ahead and split the patch.

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

655

Rather than repeatedly call hasInstrSchedModel(), why not just store the result in a bool much like you did with OptForSize..

[evandro]

Updated the patch to contain just the cost model evaluation after separating the code refactoring in a separate patch, D40090.

[junbuml]

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

697

Why don't we use <= , instead of < ?

[evandro]

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

697

Because typically decoding an instruction into multiple uops costs more in the front end of the pipeline than decoding multiple instructions into single uops.

[evandro]

Clarify the heuristics used when evaluating the costs.

[junbuml]

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

696

Can you add more comment why you do this? Is this target independent ?

[evandro]

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

696

I'm not a hardware designer, but AFAIK many targets hiccup when an instruction is decoded into more than one uop. How bad the hiccup is, if at all, does depend on the target though. Some decrease the decode bandwidth, typically by inserting a bubble whose size depends on the design. This heuristic is an attempt at mitigating the new instruction inducing such a bubble.

If the new instruction has a shorter latency, then it's chosen. One might wonder if it's still a good choice if it induces a bubble, but I could not devise a satisfying heuristic.

If the latency of the new instruction is the same as the combined latency of the both old ones, then the potential of inducing a bubble is considered. If either of the old instructions had multiple uops, then even if the new one has them too it's probably no worse than before. However, if neither of the old instructions resulted in multiple uops, the new one is chosen only if it results in fewer uops than before. One might argue that, if bubbles when decoding into multiple uops are the norm among targets, it'd be better to choose the new instruction only if it doesn't potentially induce bubbles itself.

If the new instruction has a longer latency, then it's discarded. Again, if it mitigates decode bubbles it might still be profitable, but the conditions seem hard to weigh in general.

[evandro]

Try to make the heuristics code clearer.

[evandro]

Update test cases.

[evandro]

Add diagnostic messages.

[evandro]

Ping 🔔

[junbuml]

For me, the heuristic seems pretty reasonable, but I'm not fully sure if this can make a right decision target-independently. Someone else who have better idea in hardware level may need to review this.

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

691

Isn't it possible to do just TSM.getNumMicroOps(&MIA) and remove above *SCA = TSM.resolveSchedClass(&MIA) in line 678.

707

I'm not sure if it's okay to compare values from computeInstrLatency() and getNumMicroOps() ?

[evandro]

@kristof.beyls, @mcrosier, @apinski-cavium?

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

691

Yes, it is.

707

That's what the heuristic does, as briefly explained in the debug message below.

[evandro]

Add test case for the STREAM benchmark checking the results for most targets. This way, the target maintainers can evaluate the effects of this patch on the generated code for the respective targets.

[evandro]

¡Ping! 🔔🔔

[gberry]

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

696

It seems like if you want to take this multi-uop bubble into account, you should make it explicit, and make the subtarget opt in to it.
The simple heuristic here to me would be:

if (newLatency < oldLatency)
  return true;
if (newLatency > oldLatency)
  return false;
if (newUops < oldUops)
  return true;
if (newUops > oldUops)
  return false;
if (newMultiUopPenalty < oldMultiUopPenalty)
  return true;
return false;

707

I think Jun's point was that these values are in completely different units, so comparing them doesn't make sense.

[evandro]

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

696

How do you propose that the new penalty be calculated?

707

It´s very common in process control to use one variable to effect another, as long as there´s at least an implied correlation between them.

[evandro]

Attempt at Interpreting @gberry´s suggestion.

[evandro]

Like the previous update, but favoring code size on A57, the only other target I can test on, while keeping the same performance.

[evandro]

Ping! 🔔

[evandro]

¡Ping! 🔔🔔

[gberry]

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

714

I would suggest that unless we know of a case where this makes a difference, make the simpler change of just doing

return UopDif <= 0

at this point, assuming that has the desired effect for your target. This assumes that with latency and uops being equal, a reduction in instructions could still be beneficial.

llvm/test/CodeGen/AArch64/ldst-opt.ll

1034

Are these changes intentional?

1343

Are all of these changes needed?

llvm/test/CodeGen/AArch64/machine-outliner-remarks.ll

98 ↗

(On Diff #127610)

Are these changes needed?

llvm/test/CodeGen/AArch64/stream-neon.ll

1 ↗

(On Diff #127610)

This seems like it is too large to be a lit test. Is it really testing anything that hasn't been tested already?

[evandro]

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

714

This would lead to the test below, stream-neon.ll, to fail for the Exynos targets. The reason is that, for instance, though the latency of loading a pair of quad registers with the post index addressing mode has the same latency as when using the register offset addressing mode, it is a two uop instruction, which severely limits the decode bandwidth by inserting a bubble. This is a fairly common issue in other targets too, which this heuristic tries to capture, assuming that it's better to keep the decode bandwidth for a couple single uop instructions than incurring a decode bubble for a multiple uop instruction. Except, of course, when optimizing for size.

llvm/test/CodeGen/AArch64/ldst-opt.ll

1034

Yes, though not necessary. Checking for the return is not germane to the feature being tested.

1343

Ditto.

llvm/test/CodeGen/AArch64/machine-outliner-remarks.ll

98 ↗

(On Diff #127610)

Yes, since the changes in this patch affect the resulting code for this test, unless it's optimized for size.

llvm/test/CodeGen/AArch64/stream-neon.ll

1 ↗

(On Diff #127610)

Probably not. Perhaps the same could be accomplished if ldst-opt.ll were changed to test for the same targets as this test, when the result would be same.

[evandro]

Ping! 🔔

[evandro]

¡Ping! 🔔🔔

[evandro]

Here are some results I got on a Juno board with {{-mcpu=cortex-a57}}:

SPECint2000	%
164.gzip	0.0
175.vpr	-0.4
176.gcc	0.0
177.mesa	0.8
179.art	0.1
181.mcf	-0.3
183.equake	0.9
186.crafty	0.8
188.ammp	0.0
197.parser	0.3
252.eon	-0.2
253.perlbmk	-0.2
254.gap	0.1
255.vortex	0.0
256.bzip2	0.5
300.twolf	5.1

[gberry]

I've thought about this some more and tested it out on Falkor. As currently written this change causes SIMD store instructions to not have pre/post increments folded into them, causing minor performance regressions. I have the following general reservations as well:

• does using the max latency of the load/store and add make sense given that the operations are dependent?
• does always favoring latency over number of uops (an approximation of throughput) make sense? unless the operation is on the critical path I would think not.

This combined with the assumptions about multiple uop instructions (which also is not true for Falkor), I would suggest perhaps a better approach would be a add a target-specific property that would allow you to avoid the specific opcodes that are a problem for your target.

[evandro]

In D39976#998081, @gberry wrote:

I've thought about this some more and tested it out on Falkor. As currently written this change causes SIMD store instructions to not have pre/post increments folded into them, causing minor performance regressions.

I see that they're modeled with a latency of 0 cycles and 4 uops. Are the units they need, ST and VSD, really used for 0 cycles?

I have the following general reservations as well:

• does using the max latency of the load/store and add make sense given that the operations are dependent?

They're only dependent for the pre index addressing mode. However, since the latency of the load and of the store is considerably larger even in this case, methinks that it's a sensible approximation.

• does always favoring latency over number of uops (an approximation of throughput) make sense? unless the operation is on the critical path I would think not.

In previous versions of this patch I tried to weigh both metrics, but found it difficult to come up with a satisfying heuristic. Any ideas?

This combined with the assumptions about multiple uop instructions (which also is not true for Falkor), I would suggest perhaps a better approach would be a add a target-specific property that would allow you to avoid the specific opcodes that are a problem for your target.

Perhaps the cost function could be target specific behind. Thoughts?

[evandro]

Make the heuristics processor specific.

[evandro]

Ping! 🔔

[evandro]

¡Ping! 🔔🔔

[gberry]

Would it not be simpler to just add a subtarget bool that controls whether the problematic opcodes are emitted and set it for your subtargets (similar to the way STRQroIsSlow is handled)? That way you could avoid generating them not just in this pass, but in ISel and frame lowering?

[evandro]

In D39976#1017321, @gberry wrote:

Would it not be simpler to just add a subtarget bool that controls whether the problematic opcodes are emitted and set it for your subtargets (similar to the way STRQroIsSlow is handled)? That way you could avoid generating them not just in this pass, but in ISel and frame lowering?

Methinks that the gist is to move away from features and to rely more on the cost model. In the case of this patch, it also removes the feature FeatureSlowPaired128 in D40107.

[gberry]

In D39976#1017864, @evandro wrote:

In D39976#1017321, @gberry wrote:

Would it not be simpler to just add a subtarget bool that controls whether the problematic opcodes are emitted and set it for your subtargets (similar to the way STRQroIsSlow is handled)? That way you could avoid generating them not just in this pass, but in ISel and frame lowering?

Methinks that the gist is to move away from features and to rely more on the cost model. In the case of this patch, it also removes the feature FeatureSlowPaired128 in D40107.

That seems like a worthwhile goal, but this change doesn't really seem to be accomplishing that. If the sched model is being used by a subtarget-specific heuristic, that seems like just a more roundabout way of achieving the same result for your subtarget. Is there any net effect of this change combined with D40107?

[evandro]

In D39976#1027475, @gberry wrote:

In D39976#1017864, @evandro wrote:

Methinks that the gist is to move away from features and to rely more on the cost model. In the case of this patch, it also removes the feature FeatureSlowPaired128 in D40107.

That seems like a worthwhile goal, but this change doesn't really seem to be accomplishing that. If the sched model is being used by a subtarget-specific heuristic, that seems like just a more roundabout way of achieving the same result for your subtarget. Is there any net effect of this change combined with D40107?

FeatureSlowPaired128 was just too coarse. The alternative would be to change it to something more specific, like FeatureSlowSomePaired128Sometimes, and then create yet another when for the next generation to specialize it further. Instead, querying the scheduling model seems to be a much more reasonable approach.

[evandro]

In D39976#998081, @gberry wrote:

I've thought about this some more and tested it out on Falkor. As currently written this change causes SIMD store instructions to not have pre/post increments folded into them, causing minor performance regressions.

Were a test with -mcpu=falkor added to llvm/test/CodeGen/AArch64/ldst-opt.ll, how should the checks look like?

Also, can you please address my first question in https://reviews.llvm.org/D39976#999520 ?

[evandro]

Ping! 🔔

[gberry]

In D39976#1027478, @evandro wrote:

In D39976#1027475, @gberry wrote:

In D39976#1017864, @evandro wrote:

Methinks that the gist is to move away from features and to rely more on the cost model. In the case of this patch, it also removes the feature FeatureSlowPaired128 in D40107.

That seems like a worthwhile goal, but this change doesn't really seem to be accomplishing that. If the sched model is being used by a subtarget-specific heuristic, that seems like just a more roundabout way of achieving the same result for your subtarget. Is there any net effect of this change combined with D40107?

FeatureSlowPaired128 was just too coarse. The alternative would be to change it to something more specific, like FeatureSlowSomePaired128Sometimes, and then create yet another when for the next generation to specialize it further. Instead, querying the scheduling model seems to be a much more reasonable approach.

I'm more confused now. 'FeatureSlowPaired128' controls whether certain load/store opcodes are combined to form paired load/stores. But this change prevents some load/store opcodes from having their base register increment folded in. The two seem unrelated.

I'm also concerned that this change is introducing a very specific target hook and is recomputing the same "slowness" of opcodes over and over even though it doesn't depend on the context. Perhaps a more general subtarget array of "slow" opcodes would be a better choice, which Exynos could initialize based on its scheduling model for these opcodes if you think there is going to be differences in future CPUs.

In D39976#1031443, @evandro wrote:

In D39976#998081, @gberry wrote:

I've thought about this some more and tested it out on Falkor. As currently written this change causes SIMD store instructions to not have pre/post increments folded into them, causing minor performance regressions.

Were a test with -mcpu=falkor added to llvm/test/CodeGen/AArch64/ldst-opt.ll, how should the checks look like?

I would expect this change to not change the code generated for Falkor, so whatever is generated currently

In D39976#999520, @evandro wrote:

In D39976#998081, @gberry wrote:

I've thought about this some more and tested it out on Falkor. As currently written this change causes SIMD store instructions to not have pre/post increments folded into them, causing minor performance regressions.

I see that they're modeled with a latency of 0 cycles and 4 uops. Are the units they need, ST and VSD, really used for 0 cycles?

That's not how I understand the scheduling model to work. Resources are used for 'ResourceCycles' cycles (which defaults to 1), so in this case ST and VSD are used for 1 cycle. The Latency of the store is set to 0, since it doesn't write a register, so the latency doesn't mean anything as far as I can tell. The pre/post increment version has a Latency of 3 on the first def index (i.e. the updated base register) since that is the latency of reading this new register value.

[evandro]

In D39976#1040345, @gberry wrote:

In D39976#1027478, @evandro wrote:

FeatureSlowPaired128 was just too coarse. The alternative would be to change it to something more specific, like FeatureSlowSomePaired128Sometimes, and then create yet another when for the next generation to specialize it further. Instead, querying the scheduling model seems to be a much more reasonable approach.

I'm more confused now. 'FeatureSlowPaired128' controls whether certain load/store opcodes are combined to form paired load/stores. But this change prevents some load/store opcodes from having their base register increment folded in. The two seem unrelated.

This change is more generic and flexible than FeatureSlowPaired128. This change controls not only when loads and stores are paired, but also other foldings that this pass performs, including the pre or post indexing of the offset register.

I'm also concerned that this change is introducing a very specific target hook and is recomputing the same "slowness" of opcodes over and over even though it doesn't depend on the context. Perhaps a more general subtarget array of "slow" opcodes would be a better choice, which Exynos could initialize based on its scheduling model for these opcodes if you think there is going to be differences in future CPUs.

AFAIK, the code performs table look ups, which should be fairly efficient. And, yes, just like there are differences in how well some loads and stores perform in M1 and M3, it's likely that more differences will come in their successors.

[gberry]

In D39976#1049844, @evandro wrote:

In D39976#1040345, @gberry wrote:

In D39976#1027478, @evandro wrote:

FeatureSlowPaired128 was just too coarse. The alternative would be to change it to something more specific, like FeatureSlowSomePaired128Sometimes, and then create yet another when for the next generation to specialize it further. Instead, querying the scheduling model seems to be a much more reasonable approach.

I'm more confused now. 'FeatureSlowPaired128' controls whether certain load/store opcodes are combined to form paired load/stores. But this change prevents some load/store opcodes from having their base register increment folded in. The two seem unrelated.

This change is more generic and flexible than FeatureSlowPaired128. This change controls not only when loads and stores are paired, but also other foldings that this pass performs, including the pre or post indexing of the offset register.

That's not what the code looks like it is doing. isReplacementProfitable() is only being called from mergeUpdateInsn(), which is only called when folding to form base register incrementing load/stores.

I'm also concerned that this change is introducing a very specific target hook and is recomputing the same "slowness" of opcodes over and over even though it doesn't depend on the context. Perhaps a more general subtarget array of "slow" opcodes would be a better choice, which Exynos could initialize based on its scheduling model for these opcodes if you think there is going to be differences in future CPUs.

AFAIK, the code performs table look ups, which should be fairly efficient. And, yes, just like there are differences in how well some loads and stores perform in M1 and M3, it's likely that more differences will come in their successors.

The patch also adds a function pointer call for each potential instruction pair to be optimized. I'm not that bothered by the compile-time impact of this, it just seems like too specific of a subtarget hook to be adding to me. I would appreciate hearing what other people think about this.

[evandro]

In D39976#1054542, @gberry wrote:

In D39976#1049844, @evandro wrote:

This change is more generic and flexible than FeatureSlowPaired128. This change controls not only when loads and stores are paired, but also other foldings that this pass performs, including the pre or post indexing of the offset register.

That's not what the code looks like it is doing. isReplacementProfitable() is only being called from mergeUpdateInsn(), which is only called when folding to form base register incrementing load/stores.

For now, as it's a generic enough interface to be used by many other peephole optimizations.

AFAIK, the code performs table look ups, which should be fairly efficient. And, yes, just like there are differences in how well some loads and stores perform in M1 and M3, it's likely that more differences will come in their successors.

The patch also adds a function pointer call for each potential instruction pair to be optimized. I'm not that bothered by the compile-time impact of this, it just seems like too specific of a subtarget hook to be adding to me. I would appreciate hearing what other people think about this.

So am I.

Thank you for the feedback.

[evandro]

Ping! 🔔

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineOperand.h"
 #include "llvm/CodeGen/TargetRegisterInfo.h"
+#include "llvm/CodeGen/TargetSchedule.h"
 #include "llvm/IR/DebugLoc.h"
+#include "llvm/MC/MCInstrDesc.h"
 #include "llvm/MC/MCRegisterInfo.h"
+#include "llvm/MC/MCSchedule.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"

   AliasAnalysis *AA;
   const AArch64InstrInfo *TII;
   const TargetRegisterInfo *TRI;
+  const TargetSubtargetInfo *STI;
   const AArch64Subtarget *Subtarget;
 
   // Track which registers have been modified and used.
   BitVector ModifiedRegs, UsedRegs;
 
+  // Target has a cost model.
+  bool HasCostModel;
+  TargetSchedModel TSM;
+
+  // Function is being optimized for code size.
+  bool OptForMinSize;
+
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.addRequired<AAResultsWrapperPass>();
     MachineFunctionPass::getAnalysisUsage(AU);

   bool isMatchingUpdateInsn(MachineInstr &MemMI, MachineInstr &MI,
                             unsigned BaseReg, int Offset);
 
+  // Evaluate if the new instruction is a better choice than the old ones.
+  bool isProfitable(unsigned New, MachineInstr &MIA, MachineInstr &MIB);
+
   // Merge a pre- or post-index base register update into a ld/st instruction.
   MachineBasicBlock::iterator
   mergeUpdateInsn(MachineBasicBlock::iterator I,

   }
 }
 
+bool AArch64LoadStoreOpt::isProfitable(unsigned New,
+                                       MachineInstr &MIA, MachineInstr &MIB) {
+  // Default as profitable if optimizing for size or
+  // in the absence of a cost model.
+  if (OptForMinSize || !HasCostModel)
+    return true;
+
+  const MCSchedClassDesc
+    *SCN = TSM.getMCSchedModel()->getSchedClassDesc(TII->get(New).getSchedClass()),
+    *SCA = TSM.resolveSchedClass(&MIA),
+    *SCB = TSM.resolveSchedClass(&MIB);
+  // Default as profitable if the new instr is variant or has invalid costs.
+  if (SCN->isVariant() || !SCN->isValid())
+    return true;
+
+  long LatN = TSM.computeInstrLatency(New),
+       LatA = TSM.computeInstrLatency(&MIA),
+       LatB = TSM.computeInstrLatency(&MIB);
+  long LatDif = LatN - std::max(LatA, LatB);
+  long UopN = SCN->NumMicroOps,
+       UopA = TSM.getNumMicroOps(&MIA, SCA),
+       UopB = TSM.getNumMicroOps(&MIB, SCB);
+  long UopDif = UopN - (UopA + UopB);
+
+  // The new instr is profitable if it is at least as fast when
+  // either it is simple or both other instrs are complex.
+  if (UopN <= 1 || (UopA > 1 && UopB > 1))
+    return LatDif <= 0;
+  // The new instr is profitable if it is faster than it is complex when
+  // both other instrs are simple.
+  else if (UopA <= 1 && UopB <= 1)
+    return LatDif < -UopDif;
+  // The new instr is profitable if it is at least as fast as it is complex when
+  // either other instr is complex.
+  else if (UopA > 1 || UopB > 1)
+    return LatDif <= -UopDif;
+
+  // It is not profitable.
+  return false;
+}
+
 MachineBasicBlock::iterator
 AArch64LoadStoreOpt::mergeNarrowZeroStores(MachineBasicBlock::iterator I,
                                            MachineBasicBlock::iterator MergeMI,

 
   unsigned NewOpc = IsPreIdx ? getPreIndexedOpcode(I->getOpcode())
                              : getPostIndexedOpcode(I->getOpcode());
+
+  // Evaluate if the new instruction is a better choice than both old ones.
+  if (!isProfitable(NewOpc, *I, *Update))
+    return NextI;
+
   MachineInstrBuilder MIB;
   if (!isPairedLdSt(*I)) {
     // Non-paired instruction.

   TRI = Subtarget->getRegisterInfo();
   AA = &getAnalysis<AAResultsWrapperPass>().getAAResults();
 
+  OptForMinSize = Fn.getFunction()->optForMinSize();
+
+  const TargetSubtargetInfo &STI = Fn.getSubtarget();
+  TSM.init(STI.getSchedModel(), &STI, STI.getInstrInfo());
+  // TODO: For now, only support targets with a scheduling model.  In order to
+  //       support a target that has itineraries instead, then
+  //       isProfitable() has to be modified to calculate the latency
+  //       and the number of uops.
+  HasCostModel = TSM.hasInstrSchedModel();
+
   // Resize the modified and used register bitfield trackers.  We do this once
   // per function and then clear the bitfield each time we optimize a load or
   // store.

llvm/test/CodeGen/AArch64/ldst-opt.ll

-; RUN: llc -mtriple=aarch64-linux-gnu -aarch64-enable-atomic-cfg-tidy=0 -disable-lsr -verify-machineinstrs -o - %s | FileCheck --check-prefix=CHECK --check-prefix=NOSTRICTALIGN %s
-; RUN: llc -mtriple=aarch64-linux-gnu -mattr=+strict-align -aarch64-enable-atomic-cfg-tidy=0 -disable-lsr -verify-machineinstrs -o - %s | FileCheck --check-prefix=CHECK --check-prefix=STRICTALIGN %s
+; RUN: llc -mtriple=aarch64-linux-gnu -aarch64-enable-atomic-cfg-tidy=0 -disable-lsr -verify-machineinstrs                      -o - %s | FileCheck %s --check-prefixes=CHECK,GENERIC,NOSTRICTALIGN
+; RUN: llc -mtriple=aarch64-linux-gnu -aarch64-enable-atomic-cfg-tidy=0 -disable-lsr -verify-machineinstrs -mattr=+strict-align -o - %s | FileCheck %s --check-prefixes=CHECK,GENERIC,STRICTALIGN
+; RUN: llc -mtriple=aarch64-linux-gnu -aarch64-enable-atomic-cfg-tidy=0 -disable-lsr -verify-machineinstrs -mcpu=exynos-m1      -o - %s | FileCheck %s --check-prefixes=CHECK,EXYNOS
 
 ; This file contains tests for the AArch64 load/store optimizer.
 

 %s.byte = type { i8, i8 }
 %s.halfword = type { i16, i16 }
 %s.word = type { i32, i32 }
-%s.doubleword = type { i64, i32 }
-%s.quadword = type { fp128, i32 }
+%s.doubleword = type { i64, i64 }
+%s.quadword = type { fp128, fp128 }
 %s.float = type { float, i32 }
 %s.double = type { double, i32 }
 %struct.byte = type { %padding, %s.byte }

 
 define void @load-pair-pre-indexed-word(%struct.word* %ptr) nounwind {
 ; CHECK-LABEL: load-pair-pre-indexed-word
-; CHECK: ldp w{{[0-9]+}}, w{{[0-9]+}}, [x0, #32]!
-; CHECK-NOT: add x0, x0, #32
+; GENERIC: ldp w{{[0-9]+}}, w{{[0-9]+}}, [x0, #32]!
+; GENERIC-NOT: add x0, x0, #32
+; EXYNOS: ldp w{{[0-9]+}}, w{{[0-9]+}}, [x0, #32]{{$}}
+; EXYNOS: add x0, x0, #32
 entry:
   %a = getelementptr inbounds %struct.word, %struct.word* %ptr, i64 0, i32 1, i32 0
   %a1 = load i32, i32* %a, align 4

   ret void
 }
 
+define void @load-pair-pre-indexed-doubleword(%struct.doubleword* %ptr) nounwind {
+; CHECK-LABEL: load-pair-pre-indexed-doubleword
+; GENERIC: ldp x{{[0-9]+}}, x{{[0-9]+}}, [x0, #32]!
+; GENERIC-NOT: add x0, x0, #32
+; EXYNOS: ldp x{{[0-9]+}}, x{{[0-9]+}}, [x0, #32]{{$}}
+; EXYNOS: add x0, x0, #32
+entry:
+  %a = getelementptr inbounds %struct.doubleword, %struct.doubleword* %ptr, i64 0, i32 1, i32 0
+  %a1 = load i64, i64* %a, align 8
+  %b = getelementptr inbounds %struct.doubleword, %struct.doubleword* %ptr, i64 0, i32 1, i32 1
+  %b1 = load i64, i64* %b, align 8
+  %add = add i64 %a1, %b1
+  br label %bar
+bar:
+  %c = getelementptr inbounds %struct.doubleword, %struct.doubleword* %ptr, i64 0, i32 1
+  tail call void @bar_doubleword(%s.doubleword* %c, i64 %add)
+  ret void
+}
+
+define void @store-pair-pre-indexed-doubleword(%struct.doubleword* %ptr, i64 %val) nounwind {
+; CHECK-LABEL: store-pair-pre-indexed-doubleword
+; CHECK: stp x{{[0-9]+}}, x{{[0-9]+}}, [x0, #32]!
+; CHECK-NOT: add x0, x0, #32
+entry:
+  %a = getelementptr inbounds %struct.doubleword, %struct.doubleword* %ptr, i64 0, i32 1, i32 0
+  store i64 %val, i64* %a, align 8
+  %b = getelementptr inbounds %struct.doubleword, %struct.doubleword* %ptr, i64 0, i32 1, i32 1
+  store i64 %val, i64* %b, align 8
+  br label %bar
+bar:
+  %c = getelementptr inbounds %struct.doubleword, %struct.doubleword* %ptr, i64 0, i32 1
+  tail call void @bar_doubleword(%s.doubleword* %c, i64 %val)
+  ret void
+}
+
 ; Check the following transform:
 ;
 ; add x8, x8, #16

 define void @store-pair-post-indexed-word() nounwind {
 ; CHECK-LABEL: store-pair-post-indexed-word
 ; CHECK: stp w{{[0-9]+}}, w{{[0-9]+}}, [sp], #16
-; CHECK: ret
   %src = alloca { i32, i32 }, align 8
   %dst = alloca { i32, i32 }, align 8
 

 define void @store-pair-post-indexed-doubleword() nounwind {
 ; CHECK-LABEL: store-pair-post-indexed-doubleword
 ; CHECK: stp x{{[0-9]+}}, x{{[0-9]+}}, [sp], #32
-; CHECK: ret
   %src = alloca { i64, i64 }, align 8
   %dst = alloca { i64, i64 }, align 8
 

 define void @store-pair-post-indexed-float() nounwind {
 ; CHECK-LABEL: store-pair-post-indexed-float
 ; CHECK: stp s{{[0-9]+}}, s{{[0-9]+}}, [sp], #16
-; CHECK: ret
   %src = alloca { float, float }, align 8
   %dst = alloca { float, float }, align 8
 

 define void @store-pair-post-indexed-double() nounwind {
 ; CHECK-LABEL: store-pair-post-indexed-double
 ; CHECK: stp d{{[0-9]+}}, d{{[0-9]+}}, [sp], #32
-; CHECK: ret
   %src = alloca { double, double }, align 8
   %dst = alloca { double, double }, align 8
 

   ret void
 }
 
+define void @store-pair-post-indexed-quadword() nounwind {
+; CHECK-LABEL: store-pair-post-indexed-quadword
+; GENERIC: stp q{{[0-9]+}}, q{{[0-9]+}}, [sp], #64
+; EXYNOS: str q{{[0-9]+}}, [sp]
+; EXYNOS-NEXT: str q{{[0-9]+}}, [sp, #16]
+  %src = alloca { fp128, fp128 }, align 8
+  %dst = alloca { fp128, fp128 }, align 8
+
+  %src.realp = getelementptr inbounds { fp128, fp128 }, { fp128, fp128 }* %src, i32 0, i32 0
+  %src.real = load fp128, fp128* %src.realp
+  %src.imagp = getelementptr inbounds { fp128, fp128 }, { fp128, fp128 }* %src, i32 0, i32 1
+  %src.imag = load fp128, fp128* %src.imagp
+
+  %dst.realp = getelementptr inbounds { fp128, fp128 }, { fp128, fp128 }* %dst, i32 0, i32 0
+  %dst.imagp = getelementptr inbounds { fp128, fp128 }, { fp128, fp128 }* %dst, i32 0, i32 1
+  store fp128 %src.real, fp128* %dst.realp
+  store fp128 %src.imag, fp128* %dst.imagp
+  ret void
+}
+
 ; Check the following transform:
 ;
 ; (ldr|str) X, [x20]

 
 define void @post-indexed-paired-min-offset(i64* %a, i64* %b, i64 %count) nounwind {
 ; CHECK-LABEL: post-indexed-paired-min-offset
-; CHECK: ldp x{{[0-9]+}}, x{{[0-9]+}}, [x{{[0-9]+}}], #-512
+; GENERIC: ldp x{{[0-9]+}}, x{{[0-9]+}}, [x{{[0-9]+}}], #-512
+; EXYNOS: ldp x{{[0-9]+}}, x{{[0-9]+}}, [x{{[0-9]+}}]{{$}}
 ; CHECK: stp x{{[0-9]+}}, x{{[0-9]+}}, [x{{[0-9]+}}], #-512
   br label %for.body
 for.body:

 ; scalar stores which should get merged by AArch64LoadStoreOptimizer.
 define void @merge_zr32(i32* %p) {
 ; CHECK-LABEL: merge_zr32:
-; CHECK: // %entry
-; NOSTRICTALIGN-NEXT: str xzr, [x{{[0-9]+}}]
-; STRICTALIGN-NEXT: stp wzr, wzr, [x{{[0-9]+}}]
-; CHECK-NEXT: ret
+; NOSTRICTALIGN: str xzr, [x{{[0-9]+}}]
+; STRICTALIGN: stp wzr, wzr, [x{{[0-9]+}}]
 entry:
   store i32 0, i32* %p
   %p1 = getelementptr i32, i32* %p, i32 1

 ; Same as merge_zr32 but the merged stores should also get paried.
 define void @merge_zr32_2(i32* %p) {
 ; CHECK-LABEL: merge_zr32_2:
-; CHECK: // %entry
-; NOSTRICTALIGN-NEXT: stp xzr, xzr, [x{{[0-9]+}}]
-; STRICTALIGN-NEXT: stp wzr, wzr, [x{{[0-9]+}}]
+; NOSTRICTALIGN: stp xzr, xzr, [x{{[0-9]+}}]
+; STRICTALIGN: stp wzr, wzr, [x{{[0-9]+}}]
 ; STRICTALIGN-NEXT: stp wzr, wzr, [x{{[0-9]+}}, #8]
-; CHECK-NEXT: ret
 entry:
   store i32 0, i32* %p
   %p1 = getelementptr i32, i32* %p, i32 1

 ; Like merge_zr32_2, but checking the largest allowed stp immediate offset.
 define void @merge_zr32_2_offset(i32* %p) {
 ; CHECK-LABEL: merge_zr32_2_offset:
-; CHECK: // %entry
-; NOSTRICTALIGN-NEXT: stp xzr, xzr, [x{{[0-9]+}}, #504]
-; STRICTALIGN-NEXT: str wzr, [x{{[0-9]+}}, #504]
+; NOSTRICTALIGN: stp xzr, xzr, [x{{[0-9]+}}, #504]
+; STRICTALIGN: str wzr, [x{{[0-9]+}}, #504]
 ; STRICTALIGN-NEXT: str wzr, [x{{[0-9]+}}, #508]
 ; STRICTALIGN-NEXT: str wzr, [x{{[0-9]+}}, #512]
 ; STRICTALIGN-NEXT: str wzr, [x{{[0-9]+}}, #516]
-; CHECK-NEXT: ret
 entry:
   %p0 = getelementptr i32, i32* %p, i32 126
   store i32 0, i32* %p0

 ; instruction.
 define void @no_merge_zr32_2_offset(i32* %p) {
 ; CHECK-LABEL: no_merge_zr32_2_offset:
-; CHECK: // %entry
-; NOSTRICTALIGN-NEXT: movi v[[REG:[0-9]]].2d, #0000000000000000
+; NOSTRICTALIGN: movi v[[REG:[0-9]]].2d, #0000000000000000
 ; NOSTRICTALIGN-NEXT: str q[[REG]], [x{{[0-9]+}}, #4096]
-; STRICTALIGN-NEXT: str wzr, [x{{[0-9]+}}, #4096]
+; STRICTALIGN: str wzr, [x{{[0-9]+}}, #4096]
 ; STRICTALIGN-NEXT: str wzr, [x{{[0-9]+}}, #4100]
 ; STRICTALIGN-NEXT: str wzr, [x{{[0-9]+}}, #4104]
 ; STRICTALIGN-NEXT: str wzr, [x{{[0-9]+}}, #4108]
-; CHECK-NEXT: ret
 entry:
   %p0 = getelementptr i32, i32* %p, i32 1024
   store i32 0, i32* %p0

 ; err on the side that allows for stp q instruction generation.
 define void @merge_zr32_3(i32* %p) {
 ; CHECK-LABEL: merge_zr32_3:
-; CHECK: // %entry
-; NOSTRICTALIGN-NEXT: movi v[[REG:[0-9]]].2d, #0000000000000000
+; NOSTRICTALIGN: movi v[[REG:[0-9]]].2d, #0000000000000000
 ; NOSTRICTALIGN-NEXT: stp q[[REG]], q[[REG]], [x{{[0-9]+}}]
-; STRICTALIGN-NEXT: stp wzr, wzr, [x{{[0-9]+}}]
+; STRICTALIGN: stp wzr, wzr, [x{{[0-9]+}}]
 ; STRICTALIGN-NEXT: stp wzr, wzr, [x{{[0-9]+}}, #8]
 ; STRICTALIGN-NEXT: stp wzr, wzr, [x{{[0-9]+}}, #16]
 ; STRICTALIGN-NEXT: stp wzr, wzr, [x{{[0-9]+}}, #24]
-; CHECK-NEXT: ret
 entry:
   store i32 0, i32* %p
   %p1 = getelementptr i32, i32* %p, i32 1

 ; Like merge_zr32, but with 2-vector type.
 define void @merge_zr32_2vec(<2 x i32>* %p) {
 ; CHECK-LABEL: merge_zr32_2vec:
-; CHECK: // %entry
-; NOSTRICTALIGN-NEXT: str xzr, [x{{[0-9]+}}]
-; STRICTALIGN-NEXT: stp wzr, wzr, [x{{[0-9]+}}]
-; CHECK-NEXT: ret
+; NOSTRICTALIGN: str xzr, [x{{[0-9]+}}]
+; STRICTALIGN: stp wzr, wzr, [x{{[0-9]+}}]
 entry:
   store <2 x i32> zeroinitializer, <2 x i32>* %p
   ret void

 ; Like merge_zr32, but with 3-vector type.
 define void @merge_zr32_3vec(<3 x i32>* %p) {
 ; CHECK-LABEL: merge_zr32_3vec:
-; CHECK: // %entry
-; NOSTRICTALIGN-NEXT: str xzr, [x{{[0-9]+}}]
+; NOSTRICTALIGN: str xzr, [x{{[0-9]+}}]
 ; NOSTRICTALIGN-NEXT: str wzr, [x{{[0-9]+}}, #8]
-; STRICTALIGN-NEXT: stp wzr, wzr, [x{{[0-9]+}}]
+; STRICTALIGN: stp wzr, wzr, [x{{[0-9]+}}]
 ; STRICTALIGN-NEXT: str wzr, [x{{[0-9]+}}, #8]
-; CHECK-NEXT: ret
 entry:
   store <3 x i32> zeroinitializer, <3 x i32>* %p
   ret void

 ; Like merge_zr32, but with 4-vector type.
 define void @merge_zr32_4vec(<4 x i32>* %p) {
 ; CHECK-LABEL: merge_zr32_4vec:
-; CHECK: // %entry
-; NOSTRICTALIGN-NEXT: stp xzr, xzr, [x{{[0-9]+}}]
-; STRICTALIGN-NEXT: stp wzr, wzr, [x{{[0-9]+}}]
+; NOSTRICTALIGN: stp xzr, xzr, [x{{[0-9]+}}]
+; STRICTALIGN: stp wzr, wzr, [x{{[0-9]+}}]
 ; STRICTALIGN-NEXT: stp wzr, wzr, [x{{[0-9]+}}, #8]
-; CHECK-NEXT: ret
 entry:
   store <4 x i32> zeroinitializer, <4 x i32>* %p
   ret void

 ; Like merge_zr32, but with 2-vector float type.
 define void @merge_zr32_2vecf(<2 x float>* %p) {
 ; CHECK-LABEL: merge_zr32_2vecf:
-; CHECK: // %entry
-; NOSTRICTALIGN-NEXT: str xzr, [x{{[0-9]+}}]
-; STRICTALIGN-NEXT: stp wzr, wzr, [x{{[0-9]+}}]
-; CHECK-NEXT: ret
+; NOSTRICTALIGN: str xzr, [x{{[0-9]+}}]
+; STRICTALIGN: stp wzr, wzr, [x{{[0-9]+}}]
 entry:
   store <2 x float> zeroinitializer, <2 x float>* %p
   ret void

 ; Like merge_zr32, but with 4-vector float type.
 define void @merge_zr32_4vecf(<4 x float>* %p) {
 ; CHECK-LABEL: merge_zr32_4vecf:
-; CHECK: // %entry
-; NOSTRICTALIGN-NEXT: stp xzr, xzr, [x{{[0-9]+}}]
-; STRICTALIGN-NEXT: stp wzr, wzr, [x{{[0-9]+}}]
+; NOSTRICTALIGN: stp xzr, xzr, [x{{[0-9]+}}]
+; STRICTALIGN: stp wzr, wzr, [x{{[0-9]+}}]
 ; STRICTALIGN-NEXT: stp wzr, wzr, [x{{[0-9]+}}, #8]
-; CHECK-NEXT: ret
 entry:
   store <4 x float> zeroinitializer, <4 x float>* %p
   ret void

 ; Similar to merge_zr32, but for 64-bit values.
 define void @merge_zr64(i64* %p) {
 ; CHECK-LABEL: merge_zr64:
-; CHECK: // %entry
-; CHECK-NEXT: stp xzr, xzr, [x{{[0-9]+}}]
+; CHECK: stp xzr, xzr, [x{{[0-9]+}}]
 ; CHECK-NEXT: ret
 entry:
   store i64 0, i64* %p

 ; Similar to merge_zr32, but for 64-bit values and with unaligned stores.
 define void @merge_zr64_unalign(<2 x i64>* %p) {
 ; CHECK-LABEL: merge_zr64_unalign:
-; CHECK: // %entry
-; NOSTRICTALIGN-NEXT: stp xzr, xzr, [x{{[0-9]+}}]
+; NOSTRICTALIGN: stp xzr, xzr, [x{{[0-9]+}}]
 ; STRICTALIGN: strb
 ; STRICTALIGN: strb
 ; STRICTALIGN: strb

 ; STRICTALIGN: strb
 ; STRICTALIGN: strb
 ; STRICTALIGN: strb
-; CHECK-NEXT: ret
 entry:
   store <2 x i64> zeroinitializer, <2 x i64>* %p, align 1
   ret void

 ; vector store since the zero constant vector has multiple uses.
 define void @merge_zr64_2(i64* %p) {
 ; CHECK-LABEL: merge_zr64_2:
-; CHECK: // %entry
-; NOSTRICTALIGN-NEXT: movi v[[REG:[0-9]]].2d, #0000000000000000
+; NOSTRICTALIGN: movi v[[REG:[0-9]]].2d, #0000000000000000
 ; NOSTRICTALIGN-NEXT: stp q[[REG]], q[[REG]], [x{{[0-9]+}}]
-; STRICTALIGN-NEXT: stp xzr, xzr, [x{{[0-9]+}}]
+; STRICTALIGN: stp xzr, xzr, [x{{[0-9]+}}]
 ; STRICTALIGN-NEXT: stp xzr, xzr, [x{{[0-9]+}}, #16]
-; CHECK-NEXT: ret
 entry:
   store i64 0, i64* %p
   %p1 = getelementptr i64, i64* %p, i64 1

 ; Like merge_zr64, but with 2-vector double type.
 define void @merge_zr64_2vecd(<2 x double>* %p) {
 ; CHECK-LABEL: merge_zr64_2vecd:
-; CHECK: // %entry
-; CHECK-NEXT: stp xzr, xzr, [x{{[0-9]+}}]
-; CHECK-NEXT: ret
+; CHECK: stp xzr, xzr, [x{{[0-9]+}}]
 entry:
   store <2 x double> zeroinitializer, <2 x double>* %p
   ret void

 ; Like merge_zr64, but with 3-vector i64 type.
 define void @merge_zr64_3vec(<3 x i64>* %p) {
 ; CHECK-LABEL: merge_zr64_3vec:
-; CHECK: // %entry
-; CHECK-NEXT: stp xzr, xzr, [x{{[0-9]+}}]
+; CHECK: stp xzr, xzr, [x{{[0-9]+}}]
 ; CHECK-NEXT: str xzr, [x{{[0-9]+}}, #16]
-; CHECK-NEXT: ret
 entry:
   store <3 x i64> zeroinitializer, <3 x i64>* %p
   ret void

 ; Like merge_zr64_2, but with 4-vector double type.
 define void @merge_zr64_4vecd(<4 x double>* %p) {
 ; CHECK-LABEL: merge_zr64_4vecd:
-; CHECK: // %entry
-; CHECK-NEXT: movi v[[REG:[0-9]]].2d, #0000000000000000
-; CHECK-NEXT: stp q[[REG]], q[[REG]], [x{{[0-9]+}}]
-; CHECK-NEXT: ret
+; CHECK: movi v[[REG:[0-9]]].2d, #0000000000000000
+; GENERIC-NEXT: stp q[[REG]], q[[REG]], [x{{[0-9]+}}]
+; EXYNOS-NEXT: str q[[REG]], [x{{[0-9]+}}, #16]
+; EXYNOS-NEXT: str q[[REG]], [x{{[0-9]+}}]
 entry:
   store <4 x double> zeroinitializer, <4 x double>* %p
   ret void

 ; Verify that non-consecutive merges do not generate q0
 define void @merge_multiple_128bit_stores(i64* %p) {
 ; CHECK-LABEL: merge_multiple_128bit_stores
-; CHECK: // %entry
-; NOSTRICTALIGN-NEXT: movi v[[REG:[0-9]]].2d, #0000000000000000
+; NOSTRICTALIGN: movi v[[REG:[0-9]]].2d, #0000000000000000
 ; NOSTRICTALIGN-NEXT: str q0, [x0]
 ; NOSTRICTALIGN-NEXT: stur q0, [x0, #24]
 ; NOSTRICTALIGN-NEXT: str q0, [x0, #48]
-; STRICTALIGN-NEXT: stp xzr, xzr, [x0]
+; STRICTALIGN: stp xzr, xzr, [x0]
 ; STRICTALIGN-NEXT: stp xzr, xzr, [x0, #24]
 ; STRICTALIGN-NEXT: stp xzr, xzr, [x0, #48]
-; CHECK-NEXT: ret
 entry:
   store i64 0, i64* %p
   %p1 = getelementptr i64, i64* %p, i64 1

 ; Verify that large stores generate stp q
 define void @merge_multiple_128bit_stores_consec(i64* %p) {
 ; CHECK-LABEL: merge_multiple_128bit_stores_consec
-; CHECK: // %entry
-; NOSTRICTALIGN-NEXT: movi v[[REG:[0-9]]].2d, #0000000000000000
+; NOSTRICTALIGN: movi v[[REG:[0-9]]].2d, #0000000000000000
 ; NOSTRICTALIGN-NEXT: stp q[[REG]], q[[REG]], [x{{[0-9]+}}]
 ; NOSTRICTALIGN-NEXT: stp q[[REG]], q[[REG]], [x{{[0-9]+}}, #32]
-; STRICTALIGN-NEXT: stp  xzr, xzr, [x0]
+; STRICTALIGN: stp   xzr, xzr, [x0]
 ; STRICTALIGN-NEXT: stp  xzr, xzr, [x0, #16]
 ; STRICTALIGN-NEXT: stp  xzr, xzr, [x0, #32]
 ; STRICTALIGN-NEXT: stp  xzr, xzr, [x0, #48]
-; CHECK-NEXT: ret
 entry:
   store i64 0, i64* %p
   %p1 = getelementptr i64, i64* %p, i64 1

 
 ; Check for bug 34674 where invalid add of xzr was being generated.
 ; CHECK-LABEL: bug34674:
-; CHECK: // %entry
-; CHECK-NEXT: mov [[ZREG:x[0-9]+]], xzr
+; CHECK: mov [[ZREG:x[0-9]+]], {{#0|xzr}}
 ; CHECK-DAG: stp [[ZREG]], [[ZREG]], [x0]
 ; CHECK-DAG: add x{{[0-9]+}}, [[ZREG]], #1
 define i64 @bug34674(<2 x i64>* %p) {