[PATCH] D81827: [CGP] Convert phi types

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

In D81827#2117853 <https://reviews.llvm.org/D81827#2117853>, @nikic wrote:

> InstCombine has a transform that, as far as I can see, does essentially the same as this in https://github.com/llvm/llvm-project/blob/903cf140d0118cf0d3f0f6f8967c6a20d9c5be6b/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp#L2298. I'm wondering why it is necessary to duplicate the logic in CGP. Is this because other CGP transformations only enable the transform to apply?


I had not seen that. Yes we need it to run this after the other transforms in CGP. It is really working around ISel only operating on a single basic block at a time.

I remember trying the original reproducer with the entire opt pipeline after running CGP on it, but the phi had not been converted like it needed. It looks like that version will always start with bitcast(phi(...)), and will not fan out into Uses like this one does, or catch phi(bitcast(..)) on it's own. This version seems a bit more powerful than that one (except for constants) but needs to start off at the phi to store visited's, to make sure we don't keep re-looking at the same values.

They are quite similar, but I'm not sure right now how I would combine them into a single version. That version has to work inside InstCombine, with it's limitations, and this version needs to make sure it doesn't become O(n^2) and handles all the cases we need it to. Plus I guess the inst combine version might be incorrect for X86_32?

> The InstCombine transform also had a series of issues (multiple instances of infinite loops and unprofitable transforms), so D82676 <https://reviews.llvm.org/D82676> looks familiar :) Comparing some of the logic, I'm also wondering whether this transform is legal for volatile stores (InstCombine guards by isSimple).

Good point. That's something I can do something about.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81827/new/

https://reviews.llvm.org/D81827