[PATCH] D126700: [MachineScheduler] Order more stores by ascending address

[Dave Green via Phabricator via llvm-commits]

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

Thanks. LGTM



================
Comment at: llvm/lib/Target/AArch64/AArch64.td:219
+def FeatureAscendStoreAddress : SubtargetFeature<"ascend-store-address",
+    "IsStoreAddressAscend", "false", "schedule to ascend the address of stores">;
+
----------------
Allen wrote:
> dmgreen wrote:
> > Capitalize Schedule. Maybe reword as "Schedule vector stores by ascending address".
> Thanks, apply your comment.
A q store is a store of a vector. This subtarget feature doesn't alter the scalar scores like x/w/s.


================
Comment at: llvm/lib/Target/AArch64/AArch64MachineScheduler.cpp:29
+  case AArch64::STPQi:
+    return getLdStOffsetOp(*MI).getType() == MachineOperand::MO_Immediate;
+  }
----------------
Allen wrote:
> dmgreen wrote:
> > Allen wrote:
> > > dmgreen wrote:
> > > > Why do we need to check it is an immediate?
> > > this is because some IR used to match the **str	q0, [x8, :lo12:seed_lo] ** for example, and we can't get its offset
> > > 
> > > 
> > > 
> > Why is this needed?
> if we don't check this, it will crash to use  **getImm()** get the  immediate with the above **str q0, [x8, :lo12:seed_lo] ** 
OK I see, it is a TargetFlags. That makes sense. It doesn't look like we ever generate them for STQP.


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

https://reviews.llvm.org/D126700