[PATCH] D26224: NewGVN

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

Mainly a code style review and a few random things I managed to spot. I don't know enough about GVN to go in depth.

What I did notice was that this really needs commenting more thoroughly, describing each Expression class type etc. and there needs to be more consistency with the ordering of class variables / method types.



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


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:47
+};
+class Expression {
+private:
----------------
Should this be called ExpressionBase or something?


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


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:67
+      return false;
+    if (getOpcode() == ~0U || getOpcode() == ~1U)
+      return true;
----------------
Magic numbers - replace with enum?


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:83
+  }
+  virtual void printInternal(raw_ostream &OS, bool printEType) const {
+    if (printEType)
----------------
Split the print/debug methods into their own section headed with (similar to MachineInstr.h):

```
//
// Debugging support
//
```


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:83
+  }
+  virtual void printInternal(raw_ostream &OS, bool printEType) const {
+    if (printEType)
----------------
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?


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:116
+  typedef Value **op_iterator;
+  typedef Value *const *const_ops_iterator;
+
----------------
Consistently put these next to the ops_begin() methods?


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


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


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


================
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;
----------------
static_cast<uintptr_t>(-1) ? 


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:173
+  std::vector<CongruenceClass *> CongruenceClasses;
+  unsigned NextCongruenceNum = 0;
+
----------------
Is it a good idea to leave an initializer here?


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:245
+                                           const BasicBlock *);
+  bool setBasicExpressionInfo(Instruction *, BasicExpression *,
+                              const BasicBlock *);
----------------
Don't leave this amongst the creators


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:375
+INITIALIZE_PASS_END(NewGVN, "newgvn", "Global Value Numbering", false, false)
+PHIExpression *NewGVN::createPHIExpression(Instruction *I) {
+  BasicBlock *PhiBlock = I->getParent();
----------------
newline


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:436
+    // efficient to sort by hand rather than using, say, std::sort.
+    if (Arg1 > Arg2)
+      std::swap(Arg1, Arg2);
----------------
Did you mean to compare pointer values?


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:442
+  BinaryLeader = lookupOperandLeader(Arg2, nullptr, B);
+  E->ops_push_back(BinaryLeader);
+
----------------
Tidyup?
```
E->ops_push_back(lookupOperandLeader(Arg1, nullptr, B));
E->ops_push_back(lookupOperandLeader(Arg2, nullptr, B));
```


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:459
+  if (!V)
+    return NULL;
+  if (auto *C = dyn_cast<Constant>(V)) {
----------------
```
return nullptr;
```


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:552
+    }
+
+  } else if (isa<SelectInst>(I)) {
----------------
wasted newline


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:590
+    Value *V = ConstantFoldInstOperands(I, C, *DL, TLI);
+    if (V) {
+      if (const Expression *SimplifiedE = checkSimplificationResults(E, I, V))
----------------
Remove braces:
```
if (Value *V = ConstantFoldInstOperands(I, C, *DL, TLI))
```


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:676
+  auto Operand = lookupOperandLeader(PointerOp, LI, B);
+  E->ops_push_back(Operand);
+  if (LI)
----------------
E->ops_push_back(lookupOperandLeader(PointerOp, LI, B))


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:809
+    auto *II = dyn_cast<IntrinsicInst>(EI->getAggregateOperand());
+    if (II != nullptr && EI->getNumIndices() == 1 && *EI->idx_begin() == 0) {
+      unsigned Opcode = 0;
----------------
```
if (II && EI->getNumIndices() == 1 && *EI->idx_begin() == 0) {
```


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:978
+  // congruence class.
+  if (E == NULL) {
+    // We may have already made a unique class.
----------------
```
if (E == nullptr) {
```


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1270
+  this->AA = AA;
+  this->MSSA = MSSA;
+  DL = &F.getParent()->getDataLayout();
----------------
Same named variables is asking for trouble


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1433
+  return nullptr;
+}
+struct NewGVN::ValueDFS {
----------------
newline


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1572
+  for (BasicBlock::reverse_iterator I(StartPoint); I != BB->rend();) {
+    Instruction &Inst = *I++;
+    if (!Inst.use_empty())
----------------
Why is this increment not done in the for block?


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


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


https://reviews.llvm.org/D26224