[PATCH] D26224: NewGVN

[Davide Italiano via Phabricator via llvm-commits]

davide added a subscriber: hfinkel.
davide added a comment.

In https://reviews.llvm.org/D26224#620326, @davide wrote:

> Thank you all for the comments! New revision attached.


@hfinkel : Hal, do you plan to review this?



================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:34
+
+enum ExpressionType {
+  ExpressionTypeBase,
----------------
RKSimon wrote:
> Use a short prefix not the whole enum type name: ET_Base, ..... ET_Store, ET_BasicEnd
Done.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:47
+};
+class Expression {
+private:
----------------
RKSimon wrote:
> Should this be called ExpressionBase or something?
To be honest, I prefer `Expression`.  I can change it if you feel strong about it.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:62
+      : EType(ET), Opcode(O) {}
+  virtual ~Expression();
+
----------------
RKSimon wrote:
> Keep the constructors / destructors at the top of the public block? At least not mixed in with the other methods.
Fixed.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:83
+  }
+  virtual void printInternal(raw_ostream &OS, bool printEType) const {
+    if (printEType)
----------------
RKSimon wrote:
> RKSimon wrote:
> > Split the print/debug methods into their own section headed with (similar to MachineInstr.h):
> > 
> > ```
> > //
> > // Debugging support
> > //
> > ```
> Debug print out code can be quite bulky - worth moving these to a cpp file instead of inline?
Not sure, this doesn't seem to have a terrible compile-time impact. We can move later, if needed.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:83
+  }
+  virtual void printInternal(raw_ostream &OS, bool printEType) const {
+    if (printEType)
----------------
davide wrote:
> RKSimon wrote:
> > RKSimon wrote:
> > > Split the print/debug methods into their own section headed with (similar to MachineInstr.h):
> > > 
> > > ```
> > > //
> > > // Debugging support
> > > //
> > > ```
> > Debug print out code can be quite bulky - worth moving these to a cpp file instead of inline?
> Not sure, this doesn't seem to have a terrible compile-time impact. We can move later, if needed.
yeah.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:155
+
+  static bool classof(const Expression *EB) {
+    ExpressionType et = EB->getExpressionType();
----------------
RKSimon wrote:
> Put these consistently in the same place in each class.
Done.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:205
+
+  virtual hash_code getHashValue() const {
+    return hash_combine(getExpressionType(), getOpcode(), ValueType,
----------------
RKSimon wrote:
> Add override? Probably in a few other places too.
Done, I think.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:444
+private:
+  BasicBlock *BB;
+};
----------------
RKSimon wrote:
> Put class variables consistently at the top or the bottom.
Consistently put variables at the top of the class.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:10
+/// \file
+/// This file implements the new LLVM's Global Value Numbering pass.
+///
----------------
silvas wrote:
> This file comment needs to be signficantly expanded. Remember, lots of people looking at this class might be e.g. people taking a compiler class that want to look at a "real" GVN implementation. Let's make sure come away impressed so that they will want to join LLVM!
> 
> At the very least, some citations for the relevant paper and stuff, along with summary of which exact variant of the algorithm are implemented would be good. Also, I think the high-level idea of GVN is simple enough that a high-level from-scratch description would be appropriate. I can help with writing this if you want.
> 
> In theory, we can expand this later, but when have you seen a commit improving a file-level comment? The only one I remember was in response to post-commit review asking for an improved file-level comment. So getting it right the first time is actually pretty important.
I agree. Tried to expand it a bit.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:136
+    static const Expression *getEmptyKey() {
+      uintptr_t Val = static_cast<uintptr_t>(~0U);
+      Val <<= PointerLikeTypeTraits<const Expression *>::NumLowBitsAvailable;
----------------
RKSimon wrote:
> static_cast<uintptr_t>(-1) ? 
Michael preferred ~0U, but from what I see `-1` is used everywhere else, so I'm switching back.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:459
+  if (!V)
+    return NULL;
+  if (auto *C = dyn_cast<Constant>(V)) {
----------------
RKSimon wrote:
> ```
> return nullptr;
> ```
Changed, here and everywhere else in the file.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:474
+  } else if (isa<Argument>(V) || isa<GlobalVariable>(V)) {
+#ifndef NDEBUG
+    if (I)
----------------
silvas wrote:
> Are the ifdef's necessary when all that is inside is a DEBUG?
Probably not, removing.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1270
+  this->AA = AA;
+  this->MSSA = MSSA;
+  DL = &F.getParent()->getDataLayout();
----------------
RKSimon wrote:
> Same named variables is asking for trouble
well, very spread in the new PM port, but I agree with you. I put an underscore at the beginning so that we can distinguish.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1572
+  for (BasicBlock::reverse_iterator I(StartPoint); I != BB->rend();) {
+    Instruction &Inst = *I++;
+    if (!Inst.use_empty())
----------------
RKSimon wrote:
> Why is this increment not done in the for block?
Many other passes do the same, seems like common style.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1680
+
+  for (unsigned i = 0, e = CongruenceClasses.size(); i != e; ++i) {
+    CongruenceClass *CC = CongruenceClasses[i];
----------------
RKSimon wrote:
> Use for-range loop?
> ```
> for (CongruenceClass *CC : CongruenceClasses])
> ```
Nice catch!


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1827
+      auto CurrIter = MI;
+      ++MI;
+      Value *Member = *CurrIter;
----------------
RKSimon wrote:
> Why isn't this increment done in the for block?
See comment above.


https://reviews.llvm.org/D26224