[PATCH] D29519: Add PredicateInfo utility and printing pass

[Daniel Berlin via Phabricator via llvm-commits]

dberlin marked 7 inline comments as done.
dberlin added inline comments.


================
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.
----------------
davide wrote:
> I guess this comment should go before `OriginalOp`? Or am I reading it wrong?
no, not sure how that got to that point.


================
Comment at: include/llvm/Transforms/Utils/PredicateInfo.h:109
+  PredicateBase() = delete;
+  static inline bool classof(const PredicateBase *) { return true; }
+
----------------
davide wrote:
> I guess this `classof()` is unneded, but please double-check.
We do the same thing in MemorySSA.h, but it looks like you are correct, we always allow upcasts, only requiring it for downcasts.



================
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:
> 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.
Right.
It's not really worth it for the old PM i think, but in the new PM we would want to be able to verify that either it's all valid, or all gone :)



================
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;
----------------
davide wrote:
> We have this similar pattern in many places now: `NewGVN`, `SSAUpdater`, `MemSSA` and now here. Too bad we can't actually share more.
As mentioned, i'm working on it :)


================
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() &&
----------------
davide wrote:
> Ugh, can you open a bug for this once the patch goes in?
Will do. I have to reduce it, but it's an assert here:
lib/IR/Function.cpp:537:    assert(!STyp->isLiteral() && "TODO: implement literal types");



https://reviews.llvm.org/D29519