[PATCH] D33163: [Polly] Added the list of Instructions to output in ScopInfo pass

[Tobias Grosser via Phabricator via llvm-commits]

grosser added a comment.

Hi Nandini,

thanks for the update. Find below some more comments.

I also saw that with your patch we currently get 118 unexpected failures when running 'make check-polly'. They seem to be caused because we print instructions now always. I think this might be too verbose in general. Can we make instruction printing optional and hide it behind a flag?



================
Comment at: include/polly/ScopInfo.h:1145
+  ScopStmt(Scop &parent, BasicBlock &bb, Loop *SurroundingLoop,
+           std::vector<Instruction *> bbInst);
 
----------------
In LLVM Variables start with uppercase letters.

Also, I would just call it "Instructions"


================
Comment at: include/polly/ScopInfo.h:1253
+  /// Vector for Instructions in a BB.
+  std::vector<Instruction *> bbInst;
+
----------------
Please use uppercase.


================
Comment at: include/polly/ScopInfo.h:1540
+  /// Print the instructions in ScopStmt.
+  ///
+  void printInstructions(raw_ostream &OS) const;
----------------
Add an empty line between print() and the comment that follows it.


================
Comment at: include/polly/ScopInfo.h:2030
+  void addScopStmt(BasicBlock *BB, Loop *SurroundingLoop,
+                   std::vector<Instruction *> bbInst);
 
----------------
Uppercase, variable name.


================
Comment at: lib/Analysis/ScopBuilder.cpp:635
     else {
+      std::vector<Instruction *> bbInst;
+      for (Instruction &Inst : *I->getNodeAs<BasicBlock>())
----------------
Uppercase, variable name.


================
Comment at: lib/Analysis/ScopInfo.cpp:1654
+      R(nullptr), Build(nullptr), SurroundingLoop(SurroundingLoop),
+      bbInst(bbInst) {
 
----------------
Uppercase variable name.


================
Comment at: lib/Analysis/ScopInfo.cpp:4663
   assert(BB && "Unexpected nullptr!");
-  Stmts.emplace_back(*this, *BB, SurroundingLoop);
+  Stmts.emplace_back(*this, *BB, SurroundingLoop, bbInst);
   auto *Stmt = &Stmts.back();
----------------
Uppercase variable name.


================
Comment at: test/ScopInfo/statement.ll:15
+; CHECK-NEXT:      br i1 %cmp, label %for.body, label %for.end
+; CHECK-NEXT:      br label %Stmt
+; CHECK-NEXT:      %idxprom = sext i32 %i.0 to i64
----------------
These CHECK lines do not seem correct. If I run

```
po -polly-process-unprofitable  -polly-remarks-minimal  -polly-use-llvm-names  -polly-scops -analyze < /home/grosser/Projects/polly/git/tools/polly/test/ScopInfo/statement.ll
```

I get:

```
    Statements {
    	Stmt_Stmt
            Domain :=
                { Stmt_Stmt[i0] : 0 <= i0 <= 1023 };
            Schedule :=
                { Stmt_Stmt[i0] -> [i0] };
            MustWriteAccess :=	[Reduction Type: NONE] [Scalar: 0]
                { Stmt_Stmt[i0] -> MemRef_A[i0] };
            MustWriteAccess :=	[Reduction Type: NONE] [Scalar: 0]
                { Stmt_Stmt[i0] -> MemRef_B[i0] };
            Instructions {
      %idxprom = sext i32 %i.0 to i64
      %arrayidx = getelementptr inbounds i32, i32* %A, i64 %idxprom
      store i32 %i.0, i32* %arrayidx, align 4
      %idxprom1 = sext i32 %i.0 to i64
      %arrayidx2 = getelementptr inbounds i32, i32* %B, i64 %idxprom1
      store i32 %i.0, i32* %arrayidx2, align 4
      br label %for.inc
    }
    }
```

These four "CHECK" lines do not make sense:

```
; CHECK-NEXT:      %i.0 = phi i32 [ 0, %entry ], [ %add, %for.inc ]
​; CHECK-NEXT:      %cmp = icmp slt i32 %i.0, 1024
​; CHECK-NEXT:      br i1 %cmp, label %for.body, label %for.end
; CHECK-NEXT:      br label %Stmt
```

Also, the indentation seems off.


https://reviews.llvm.org/D33163