[PATCH] D30416: [BitfieldShrinking] Shrink Bitfields load/store when the bitfields are legal to access independently

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

I'll find some time to look at the core algorithm later.



================
Comment at: include/llvm/Target/TargetLowering.h:1908
+  virtual bool isNarrowingExpensive(EVT /*VT1*/, EVT /*VT2*/) const {
+    return true;
+  }
----------------
I'm not sure I see the point of this hook.  Every in-tree target has cheap i8 load/store and aligned i16 load/store operations.  And we have existing hooks to check support for misaligned operations.

If there's some case I'm not thinking of, please add an example to the comment.


================
Comment at: lib/CodeGen/MemAccessShrinking.cpp:52
+///   and more complex control flow. That is why MemorySSA is used here. It is
+///   scalable and precise for the safety check especially when we tries to
+///   insert a shrinked load in the block which is many blocks away from its
----------------
"try", not "tries".


================
Comment at: lib/CodeGen/MemAccessShrinking.cpp:1014
+          else
+            CandidatesToErase.insert(OpI);
+        }
----------------
If an instruction has no uses and isn't trivially dead, we're never going to erase it; no point to adding it to CandidatesToErase.


================
Comment at: lib/CodeGen/MemAccessShrinking.cpp:1027
+    } while (!DeadInsts.empty());
+  }
+  return Changed;
----------------
Should we clear CandidatesToErase here, as opposed to modifying it inside the loop?


Repository:
  rL LLVM

https://reviews.llvm.org/D30416