[PATCH] D77813: [ARM] Distribute MVE post-increments

[Sjoerd Meijer via Phabricator via llvm-commits]

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

Yeah, nice one, LGTM too. 
Just some extra nits that don't need another review.



================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:1389
+
+  #define DOMVE(X) case ARM::X: return ARM::X##_post
+  DOMVE(MVE_VLDRBS16);
----------------
nit: not a huge fan of macros, also if you grep for an opcode this makes it more difficult to find, but okay, probably fine here. Perhaps only a more descriptive name than DOMVE? 


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2509
+// Get the Base register operand index from the memory access MachineInst if we
+// should attempt to distrubute postinc on it. Return -1 if not of a valid
+// instruction type. If it returns an index, it is assumed that instruction is a
----------------
typo: distrubute


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2579
+  // Other accesses after BaseAccess that will need to be updated to use the
+  // postin value
+  SmallPtrSet<MachineInstr *, 8> OtherAccesses;
----------------
typo: postin -> postinc


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2595
+    int64_t Offset = Use.getOperand(BaseOp + 1).getImm();
+    if (Offset == 0)
+      BaseAccess = &Use;
----------------
nit: since we don't use Offset:

   if (!Use.getOperand(BaseOp + 1).getImm())


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2630
+
+  // And make sure that -ve increments can be added to all other offsets after
+  // the BaseAccess. We rely on either dominates(BaseAccess, OtherAccess) or
----------------
nit: I might be missing something obvious,  but I don't know what's meant with "-ve increments"


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2677
+
+bool ARMPreAllocLoadStoreOpt::DistributeIncrements() {
+  bool Changed = false;
----------------
dmgreen wrote:
> SjoerdMeijer wrote:
> > Do we always want to run this? I.e., it looks like this is always run as a first step and can generate negative offsets, so was wondering if that can impact code-size.
> I think so far it will only end up removing instructions, so can always be run. It should only ever make things smaller. And I don't believe there is a performance impact in MVE of a postinc load into an offset load.
> 
> With normal loads like LDR, which I will hopefully add in another patch, there might be some times when it is better to create LDRD or LDM and we have to be more careful. That's not something that we have to worry about yet though.
okidoki, cheers


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

https://reviews.llvm.org/D77813