[PATCH] D29519: Add PredicateInfo utility and printing pass

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

Some comments inline. I want to take another look, but this is coming along very nicely.



================
Comment at: include/llvm/Transforms/Utils/PredicateInfo.h:103-105
+  // The original operand before we renamed it.
+  // This can be use by passes, when destroying predicateinfo, to know
+  // whether they can just drop the intrinsic, or have to merge metadata.
----------------
I guess this comment should go before `OriginalOp`? Or am I reading it wrong?


================
Comment at: include/llvm/Transforms/Utils/PredicateInfo.h:109
+  PredicateBase() = delete;
+  static inline bool classof(const PredicateBase *) { return true; }
+
----------------
I guess this `classof()` is unneded, but please double-check.


================
Comment at: include/llvm/Transforms/Utils/PredicateInfo.h:125
+  PredicateAssume() = delete;
+  static inline bool classof(const PredicateAssume *) { return true; }
+  static inline bool classof(const PredicateBase *PB) {
----------------
I guess this `classof()` is unneded, but please double-check.


================
Comment at: include/llvm/Transforms/Utils/PredicateInfo.h:146
+
+  static inline bool classof(const PredicateBranch *) { return true; }
+  static inline bool classof(const PredicateBase *PB) {
----------------
I guess this is unneeded, isn't it? (please double check).


================
Comment at: lib/Transforms/Utils/PredicateInfo.cpp:585
+  OS << "PredicateInfo for function: " << F.getName() << "\n";
+  make_unique<PredicateInfo>(F, DT, AC)->print(OS);
+
----------------
The new PM port doesn't do verification of PredicateInfo. Any reason why? (I think it's very useful as an increasing number of people is now playing with the new pass manager).


================
Comment at: lib/Transforms/Utils/PredicateInfo.cpp:585
+  OS << "PredicateInfo for function: " << F.getName() << "\n";
+  make_unique<PredicateInfo>(F, DT, AC)->print(OS);
+
----------------
davide wrote:
> The new PM port doesn't do verification of PredicateInfo. Any reason why? (I think it's very useful as an increasing number of people is now playing with the new pass manager).
Oh, I see you split in two different passes in the new PM. Sounds good.


================
Comment at: lib/Transforms/Utils/PredicateInfo.cpp:65-73
+struct ValueDFS {
+  int DFSIn = 0;
+  int DFSOut = 0;
+  unsigned int LocalNum = LN_Middle;
+  PredicateBase *PInfo = nullptr;
+  // Only one of Def or Use will be set.
+  Value *Def = nullptr;
----------------
We have this similar pattern in many places now: `NewGVN`, `SSAUpdater`, `MemSSA` and now here. Too bad we can't actually share more.


================
Comment at: lib/Transforms/Utils/PredicateInfo.cpp:141-145
+    if (ADef) {
+      AInst = cast<Instruction>(ADef);
+    } else {
+      AInst = cast<Instruction>(A.Use->getUser());
+    }
----------------
Remove braces.


================
Comment at: lib/Transforms/Utils/PredicateInfo.cpp:156
+    else {
+      //      auto *OBB = new OrderedBasicBlock(BB);
+      auto Result = OBBMap.insert({BB, make_unique<OrderedBasicBlock>(BB)});
----------------
commented code, remove.


================
Comment at: lib/Transforms/Utils/PredicateInfo.cpp:223-224
+  //
+  // FIXME: LLVM crashes trying to create an intrinsic declaration of some
+  // pointer to function types that return structs, so we avoid them.
+  if ((isa<Instruction>(Op0) || isa<Argument>(Op0)) && !Op0->hasOneUse() &&
----------------
Ugh, can you open a bug for this once the patch goes in?


https://reviews.llvm.org/D29519