[PATCH] D29578: Propagate debug info for Phi node in SSAUpdater

Mon Feb 20 05:05:14 PST 2017

dberlin added a comment.

In https://reviews.llvm.org/D29578#681419, @samparker wrote:

> Hi Daniel,
>
> Thanks for your comments. I had hoped that performing this in the SSA updater would prevent this bug from appearing in other passes that update/replace the Phi nodes.

I agree with your general idea and thought that it would be nice to solve once and for all.
However,  if you wanted to do that, you'd have to do two things:
A. Demonstrate it was a general problem that needs solving (Ie there were other places with the same issue). This is just because we don't want to make a general thing to solve problems that may not actually be general.
B. Demonstrate you could do so quickly and efficiently in a general way.  Especially in SSAUpdater, which gets used a *lot*.
If you want to do both, great.
So far, i think B is your bigger issue.  Your approach so far is N^2, and it seems hard to make it not N^2 with the API SSAUpdater provides.  I've got an SSAUpdater rewrite outstanding, but i didn't change the API.

If you demonstrate it's a larger problem, i'm happy to help think about what kind of API might allow us to solve this issue in better time bounds.
But such a thing may not be theoretically possible. In which case, i'd argue we should just bite the bullet and fix passes that have this issue, and do so in a way that is faster (and then add testcases to make sure nobody breaks it)

> Do you think there's a way to achieve this in a manner that isn't local to loop rotate?

I'd need more data. IE at least some other examples where passes are breaking this

> And by stashing and replacing, will getMetadata and setMetadata be okay for this purpose?

Sure

> Would I still not have to update the debug value to reflect the value of the new phi node?

You would, but again, in the case of loop rotate, you know you have a 1:1 mapping between old and new.

> Thanks again,
> sam

https://reviews.llvm.org/D29578