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

Tue Mar 8 16:59:56 PST 2016

qcolombet added a comment.

Hi Jonas,

I will check this is NFC for AArch64 purposes, but that looks good generally speaking.
At his point, I only have comments on the comments :).

I'll try to get back to you by the end of the week.

Cheers,
-Quentin


================
Comment at: include/llvm/CodeGen/ReachingPhysDefs.h:39
@@ +38,3 @@
+  void dump();
+};
+
----------------
I think the common practice when defining typedefs is to put them in the main class of the header file.
Indeed, we do not want to have symbols collision and a class provides a sort of namespace.

================
Comment at: include/llvm/CodeGen/ReachingPhysDefs.h:82
@@ +81,3 @@
+  /// Dummy uses are recorded using this MI, during local analysis.
+  MachineInstr *DummyOp;
+
----------------
I am not sure "recorded" is the right word.
IIRC DummyOp is used to simulate that a definition is live-out a block when we actually do not do the full analysis.
In other word, DummyOp is a sentinel that ensures the reaching def analysis is conservatively correct when BasicBlockScopeOnly is used.

================
Comment at: include/llvm/CodeGen/ReachingPhysDefs.h:87
@@ +86,3 @@
+  /// Out: a set per color of definitions that reach the
+  ///      out boundary of this block.
+  /// In: Same as Out but for in boundary.
----------------
Now that I look back at this comment, although I know what I meant, it is really hard to understand.
What about:
A set of definitions, one per color, that reaches the out boundary of this block.
A given color may have more that one definition reaching a basic block boundary because of live-in/live-through definitions.

================
Comment at: include/llvm/CodeGen/ReachingPhysDefs.h:97
@@ +96,3 @@
+
+  /// Given a couple (MBB, reg) get the corresponding set of
+  /// instructions from the given "sets".
----------------
While we are here, we can fix the comments ;).
s/couple/pair/

================
Comment at: include/llvm/CodeGen/ReachingPhysDefs.h:113
@@ +112,3 @@
+  //// Virtual methods that make it possible to override common analysis
+  //// results in a a specialized transformation context.
+
----------------
typo: a a

================
Comment at: include/llvm/CodeGen/ReachingPhysDefs.h:113
@@ +112,3 @@
+  //// Virtual methods that make it possible to override common analysis
+  //// results in a a specialized transformation context.
+
----------------
qcolombet wrote:
> typo: a a
You may use the doxygen grouping comment: \@ { ... \@}

================
Comment at: include/llvm/CodeGen/ReachingPhysDefs.h:116
@@ +115,3 @@
+  /// Process the use operands of/MI.
+  virtual void processMIUses(MachineInstr &MI);
+  /// Given MO, return a used register if any.
----------------
/ -> \p ?

================
Comment at: include/llvm/CodeGen/ReachingPhysDefs.h:116
@@ +115,3 @@
+  /// Process the use operands of/MI.
+  virtual void processMIUses(MachineInstr &MI);
+  /// Given MO, return a used register if any.
----------------
qcolombet wrote:
> / -> \p ?
Since this can be overloaded, I would be more verbose about what this processing is supposed to do.

================
Comment at: include/llvm/CodeGen/ReachingPhysDefs.h:118
@@ +117,3 @@
+  /// Given MO, return a used register if any.
+  virtual unsigned processMIUseMO(MachineOperand &MO);
+  /// Process MI clobbsers (regmask)
----------------
What is returned if case of no use?
I am guessing 0, but it needs to be documented.

================
Comment at: include/llvm/CodeGen/ReachingPhysDefs.h:119
@@ +118,3 @@
+  virtual unsigned processMIUseMO(MachineOperand &MO);
+  /// Process MI clobbsers (regmask)
+  virtual void processMIClobbers(MachineInstr &MI);
----------------
Typo: clobbsers -> clobbers

================
Comment at: include/llvm/CodeGen/ReachingPhysDefs.h:119
@@ +118,3 @@
+  virtual unsigned processMIUseMO(MachineOperand &MO);
+  /// Process MI clobbsers (regmask)
+  virtual void processMIClobbers(MachineInstr &MI);
----------------
qcolombet wrote:
> Typo: clobbsers -> clobbers
Same comment as processMIUses: document what is the expected processing here.

================
Comment at: include/llvm/CodeGen/ReachingPhysDefs.h:126
@@ +125,3 @@
+  /// maps.
+  virtual bool rememberClobberingMI() { return false; }
+
----------------
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.

================
Comment at: include/llvm/CodeGen/ReachingPhysDefs.h:130
@@ +129,3 @@
+  /// doing local analysis.
+  virtual bool recordDummyUses() { return true; }
+
----------------
Does this have to do with DummyOp?
I think it was about adding DummyOp to the final reachable use/def maps, doesn't it?

================
Comment at: include/llvm/CodeGen/ReachingPhysDefs.h:183
@@ +182,3 @@
+
+  /// Do a reaching defs analyzis on a given set of phys regs. If a
+  /// single reg is given, it is added.
----------------
analysis.

================
Comment at: include/llvm/CodeGen/ReachingPhysDefs.h:185
@@ +184,3 @@
+  /// single reg is given, it is added.
+  bool analyze(unsigned singleReg = 0);
+
----------------
Be more explicit about "it is added".
I am guessing it means "it is added to the set of registers handled by the algorithm".

================
Comment at: lib/CodeGen/ReachingPhysDefs.cpp:35
@@ +34,3 @@
+/// for this couple and returned.
+SetOfMachineInstr &ReachingPhysDefs::
+getSet(BlockToSetOfInstrsPerColor &sets, const MachineBasicBlock &MBB,
----------------
Either move the doxygen comment to the header or make this a plain comment.
I don't think doxygen likes to have multiple comments for the same method, plus it is a pain when we need to update them.

This remark stands for all methods in the file :).

================
Comment at: lib/CodeGen/ReachingPhysDefs.cpp:49
@@ +48,3 @@
+/// Given a couple (reg, MI) get the corresponding set of instructions
+/// from the the given "sets".  This is used to get the uses record in
+/// sets of a definition identified by MI and reg, i.e., MI defines
----------------
recorded.

================
Comment at: lib/CodeGen/ReachingPhysDefs.cpp:93
@@ +92,3 @@
+void ReachingPhysDefs::
+pruneMultipleDefs() {
+  for (unsigned CurReg = 0; CurReg < nbRegs(); ++CurReg) {
----------------
The formatting looks weird to me.
clang-format?

================
Comment at: lib/Target/AArch64/AArch64CollectLOH.cpp:127
@@ -125,3 +126,3 @@
 
-#define DEBUG_TYPE "aarch64-collect-loh"
+#define DEBUG_TYPE "reachingdefs"
 
----------------
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.

================
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) {
----------------
This const could remain, right?


http://reviews.llvm.org/D17257



