[PATCH] D28193: NewGVN: Add UnknownExpression and create them for things we can't symbolize. Kill fragile machinery for handling null expressions.

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

This generally feels like an improvement, but I have a question (inline) about handling of terminators.
I can help reducing the testcase if you provide the original one (not sure which PR this is associated with, if any).



================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:586
+  virtual bool equals(const Expression &Other) const override {
+    const UnknownExpression &OU = cast<UnknownExpression>(Other);
+    return Inst == OU.Inst;
----------------
`auto &` probably as `cast<>` makes the type obvious.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:644-648
+const UnknownExpression *NewGVN::createUnknownExpression(Instruction *I) {
+  auto *E = new (ExpressionAllocator) UnknownExpression(I);
+  E->setOpcode(I->getOpcode());
+  return E;
+}
----------------
I think we may want to add doxygen documentation to every `create*` function, but probably that can be done separately in a single pass (if you agree, of course).


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1015-1032
-  // Expressions we can't symbolize are always in their own unique
-  // congruence class.  FIXME: This is hard to perfect. Long term, we should try
-  // to create expressions for everything. We should add UnknownExpression(Inst)
-  // or something to avoid wasting time creating real ones.  Then the existing
-  // logic will just work.
-  if (E == nullptr) {
-    // We may have already made a unique class.
----------------
Glad to see this going away =)


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1392-1397
+    // Unlike other terminators, invokes can return values. Just give them all
+    // unique return values.
+    if (isa<InvokeInst>(I)) {
+      auto *Symbolized = createUnknownExpression(I);
+      performCongruenceFinding(I, Symbolized);
+    }
----------------
There's another terminator which can return values, i.e. `catchswitch`. Maybe we should handle it specially here as well? http://llvm.org/docs/LangRef.html#catchswitch-instruction


https://reviews.llvm.org/D28193