[PATCH] D17257: New utility class ReachingPhysDefs for post-ra analysis.

[Jonas Paulsson via llvm-commits]

jonpa added inline comments.

================
Comment at: include/llvm/CodeGen/ReachingPhysDefs.h:126
@@ +125,3 @@
+  /// maps.
+  virtual bool rememberClobberingMI() { return false; }
+
----------------
qcolombet wrote:
> Could you document why this is useful?
> My vague recollection seems to indicate that this is useful for the related instructions to appear in the final reachable use/def.
This is only used by the ADRP analysis, so I am guessing this is because that class reverses semantics so that ADRP defs are uses. I am not sure of the details...

================
Comment at: lib/CodeGen/ReachingPhysDefs.cpp:301
@@ +300,3 @@
+void ReachingPhysDefs::reachingDefAlgorithm() {
+  bool HasChanged;
+  do {
----------------
dberlin wrote:
> You should iterate this in the right order to converge faster.
> 
> Look at LiveDebugValues.cpp::ExtendRanges
> 
> You should be able to copy and reuse the relevant worklist code there.
> 
Yes, interesting, thanks. I copied it and thought perhaps this could even be some algorithm class with a virtual method for handling of MBB?


================
Comment at: lib/Target/AArch64/AArch64CollectLOH.cpp:127
@@ -125,3 +126,3 @@
 
-#define DEBUG_TYPE "aarch64-collect-loh"
+#define DEBUG_TYPE "reachingdefs"
 
----------------
qcolombet wrote:
> I am fine in keeping the aarch64-collect-loh, though I see why you are doing it.
> That will be indeed cumbersome to debug this pass without the reachingdefs output.
> That behind said, it is probably a testimony to support more than one debug-only switch.
I have also found other cases where it would be useful. For instance, in [Target]InstrInfo.cpp target implementations, some methods should get debug output along with the passes where they are called from.

================
Comment at: lib/Target/AArch64/AArch64CollectLOH.cpp:527
@@ -698,3 +526,3 @@
 
-  const MachineInstr *Def = *UseToDefs.find(Instr)->second.begin();
+  MachineInstr *Def = *UseToDefs.find(Instr)->second.begin();
   if (Def->getOpcode() != AArch64::ADRP) {
----------------
qcolombet wrote:
> This const could remain, right?
No, in order to use this class when experimenting with SystemZElimCompare, in such a way as to change CC-users, I thought it made sense to remove the const specifier.

If it would remain, I guess users of the algorithm would have to do a const_cast when they wanted to change a user / def?



http://reviews.llvm.org/D17257