[PATCH] D125377: [AArch64] Order STP Q's by ascending address

[Florian Hahn via Phabricator via llvm-commits]

fhahn accepted this revision.
fhahn added a comment.
This revision is now accepted and ready to land.

LGTM, I think that should be fine. ordering by ascending address is likely also slightly more readable for our users.



================
Comment at: llvm/lib/Target/AArch64/AArch64MachineScheduler.cpp:18
+
+  if (Cand.isValid()) {
+    MachineInstr *Instr0 = TryCand.SU->getInstr();
----------------
might be good to reduce an early exit here if the candidate isn't valid to reduce the nesting level below. 


================
Comment at: llvm/lib/Target/AArch64/AArch64MachineScheduler.cpp:22
+    // When dealing with two STPqi's.
+    if (Instr0 && Instr1 && Instr0->getOpcode() == Instr1->getOpcode () &&
+        Instr0->getOpcode() == AArch64::STPQi)
----------------
nit: IMO it would be easier to follow the code if there would be a comment upfront describing what the whole block does, rather than interleaving the comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64MachineScheduler.h:33
+#endif
+
----------------
nit: Stray newline


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetMachine.cpp:484
       // pseudos then (v. addPreSched2()).
-      ScheduleDAGMI *DAG = createGenericSchedPostRA(C);
       DAG->addMutation(createAArch64MacroFusionDAGMutation());
       return DAG;
----------------
avieira wrote:
> fhahn wrote:
> > Might make sense to move the code from `AArch64MacroFusion.cpp` to the new `"AArch64MachineScheduler.h"`
> I don't have strong feelings about this, but other targets seem to keep mutations and MachineScheduler definitions separately. Could put them all in the same file if you prefer or leave it as is... I do assume you meant merge AArch64MacroFusion.{h,cpp} with respectively AArch64MachineScheduler.{h,cpp}.
>  I do assume you meant merge AArch64MacroFusion.{h,cpp} with respectively AArch64MachineScheduler.{h,cpp}.

Yeah, moving the stuff from `AArch64MacroFusion` to `AArch64MachineScheduler`. In any case that's something that should be done one as follow-up.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125377/new/

https://reviews.llvm.org/D125377