[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 accepted this revision.
davide added a comment.
This revision is now accepted and ready to land.

This looks good with the comment on terminators inline addressed. I wouldn't worry too much about adding a test for `catchswitch`, but up to you.



================
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;
+}
----------------
dberlin wrote:
> davide wrote:
> > 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).
> Sure.
> What do you want it to say?
> 
> There are only a few that have any special behavior.  :)
> 
My bad, I was thinking about the `perform*` class of function. Some of them are commented (sometimes the comment is trivial) and some of them are not. I'd rather have some uniformity there. This one is good, of course, as it doesn't have any specific behaviour.


================
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);
+    }
----------------
dberlin wrote:
> davide wrote:
> > 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
> I think i can just change the test to be about whether the terminator is a void type.
> I'll check.
> 
Yes, I think that's a more robust way of handling (in case somebody introduces a new terminator etc..)


https://reviews.llvm.org/D28193