[PATCH] D76235: [ARM][LowOverheadLoops] Add checks for narrowing

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

I am now up to date with this, and don't have any questions about the logic, that looks good.
I would like to go over it once more when (some of) the nits have been addressed as I would like to get an overview how things read and look as this introduces quite a few new things and concepts.



================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:538
+
+static bool retainsPreviousValue(const MachineInstr &MI) {
+  switch (MI.getOpcode()) {
----------------
samparker wrote:
> SjoerdMeijer wrote:
> > Same request here: what is `retainsPreviousValue`, and why these opcodes?
> All these instructions operate upon half a lane of the source register, writing to the destination, but they also leave the other half of the destination register untouched. The reference manual uses this sentence: 'The other half of the destination vector element retains its previous value'.
Thanks for pointing this out.

Just a nit/suggestion for the function name then: 
`retainsPreviousValue` -> `laneRetainsPreviousValue`

A few questions about the list below. Is this list meant to be complete? From looking at the ArmARM a bit more for this behaviour, it looks like there are more, the first search result e.g.  showed VQMOVN. Are we for example not interested in this one? Also curious if it would be worth describing this property in a different way, or if this switch will do.



================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:583
+
+static bool alwaysZero(MachineInstr &MI, const TargetRegisterClass *QPRs,
+                       const ReachingDefAnalysis &RDA,
----------------
SjoerdMeijer wrote:
> Same here
nit: perhaps something along the lines of: `alwaysZero` -> `laneAlwaysZero`


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:617
   // A masked load, whether through VPT or tail predication, will write zeros
   // to any of the falsely predicated bytes. So, from the loads, we know that
   // the false lanes are zeroed and here we're trying to track that those false
----------------
nit: bytes -> lanes?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:625
   // loop and the tail-predicated form too. Because of this, we can insert
   // loads, stores and other predicated instructions into our KnownFalseZeros
   // set and build from there.
----------------
nit: KnownFalseZeros -> Predicated?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:629
+  SetVector<MachineInstr *> Unknown;
+  SmallPtrSet<MachineInstr *, 4> AlwaysFalseZeros;
+  SmallPtrSet<MachineInstr *, 4> Predicated;
----------------
nit: perhaps something with "lane" in it:
- AlwaysFalseLaneZero, or
- FalseLaneZero, or
- FalsePredLaneZero


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

https://reviews.llvm.org/D76235