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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:521
 
+static bool canGenerateNonZeros(const MachineInstr &MI) {
+  switch (MI.getOpcode()) {
----------------
This is becoming a very tricky pass (if it wasn't already that), for a bunch of different reason: the analysis, the transformations, etc. To keep things readable, I am now going to nitpick on comments. I believe there is one school of thought that code should be self-documenting, and comments will always be out of date or misleading, which I think I can mostly agree with. But here in this case, this function, that checks certain opcodes and returning true/false to a function named `canGenerateNonZeros` doesn't tell me much. So my request is here is if you can define what exactly canGenerateNonZeros mean, and why these opcodes.
 


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:538
+
+static bool retainsPreviousValue(const MachineInstr &MI) {
+  switch (MI.getOpcode()) {
----------------
Same request here: what is `retainsPreviousValue`, and why these opcodes?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:583
+
+static bool alwaysZero(MachineInstr &MI, const TargetRegisterClass *QPRs,
+                       const ReachingDefAnalysis &RDA,
----------------
Same here


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:588
+    return false;
+ // Look at its register uses to see if it only can only receive zeros
+ // into its false lanes which would then produce zeros. Also check that
----------------
Probably this comment describes the function's purpose, so can be moved up.


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

https://reviews.llvm.org/D76235