[PATCH] D88209: [ARM] Check for LSTP side-effects.

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Looks good, but ignoring the nits I have one question inlined that asks about explaining why we are doing this, and am interested to have a read first.



================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:571
 
+  // Could inserting the LSTP cause some unintended affects?
+  auto cannotInsertLSTPBetween = [](MachineInstr *Begin,
----------------
nit: first thought it was a typo, but it isn't of course....perhaps:   LSTP -> [W|D]LSTP



================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:571
 
+  // Could inserting the LSTP cause some unintended affects?
+  auto cannotInsertLSTPBetween = [](MachineInstr *Begin,
----------------
SjoerdMeijer wrote:
> nit: first thought it was a typo, but it isn't of course....perhaps:   LSTP -> [W|D]LSTP
> 
can you be a bit more specific, i.e. what the exact conditions are when we can't insert it and why we are doing this?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:572
+  // Could inserting the LSTP cause some unintended affects?
+  auto cannotInsertLSTPBetween = [](MachineInstr *Begin,
+                                    MachineInstr *End) {
----------------
nit: perhaps cannotInsertLSTPBetween -> cannotInsertWDLSTPBetween


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88209