[PATCH] D29295: Move core RDF files from lib/Target/Hexagon to CodeGen

Matthias Braun via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Aug 15 11:46:39 PDT 2017


MatzeB resigned from this revision.
MatzeB added a comment.

This is a huge chunk of code to review, and to really form an opinion about it I believe it needs to be used in practice.
Overal the code looks fine to me, but I didn't dive in too deeply. I wonder if inventing a new allocator was really necessary here or what the point of TargetOperandInfo is (I would expect the register and regmask operand to have a specific clear meaning and this looks like a way to have targets override that?).

But all in all I'll leave review to whoever plans to use this code.



================
Comment at: include/llvm/CodeGen/RDFGraph.h:10
+//
+// Target-independent, SSA-based data flow graph for register data flow (RDF)
+// for a non-SSA program representation (e.g. post-RA machine code).
----------------
MatzeB wrote:
> should be `/// \file`
seems unchanged.


================
Comment at: include/llvm/CodeGen/RDFGraph.h:926-941
+  template <typename T> struct Print;
+  template <typename T>
+  raw_ostream &operator<< (raw_ostream &OS, const Print<T> &P);
+
+  template <typename T>
+  struct Print {
+    Print(const T &x, const DataFlowGraph &g) : Obj(x), G(g) {}
----------------
You could use the `Printable` helper class here.


================
Comment at: include/llvm/CodeGen/RDFLiveness.h:23-28
+  class MachineBasicBlock;
+  class MachineFunction;
+  class MachineRegisterInfo;
+  class TargetRegisterInfo;
+  class MachineDominatorTree;
+  class MachineDominanceFrontier;
----------------
no indentation for namespaces, also a strange de-dent when the rdf namespace starts. Similar in the other files.


================
Comment at: lib/CodeGen/RDFLiveness.cpp:19
+//
+// Dibyendu Das, Ramakrishna Upadrasta, Benoit Dupont de Dinechin.
+// "Efficient Liveness Computation Using Merge Sets and DJ-Graphs."
----------------
kparzysz wrote:
> MatzeB wrote:
> > There is an alternative algorithm floating around, for example https://reviews.llvm.org/D28934
> > 
> > Which I usually prefer because it is simpler and doesn't require dominance tree construction. But I may be biased here :)
> The algorithm you linked to is for building/updating SSA, this one is for calculating block live-ins from existing SSA.
Sorry, indeed.


Repository:
  rL LLVM

https://reviews.llvm.org/D29295





More information about the llvm-commits mailing list