[PATCH] D52061: [RegisterCoalescer] Only look at main ranges in valuesIdentical/followCopyChain

[Tim Renouf via Phabricator via llvm-commits]

tpr added a comment.

Hi Quentin

Firstly, it seems I now do not have a test case for this. The test that previously failed with the https://reviews.llvm.org/D51848 fix but passed with this fix now seems to pass all the time. I don't know what changed.

Anyway, the situation I used to have with that test was something like this:

  100B %23:sub0 = ...
  110B %23:sub1 = ...
  120B %23:sub2 = ...
  130B %42 = COPY %23
  140B %51 = COPY %23

Are %42:130r and %51:140r identical?

In the main range, following the copy chain for both ends up at the 120r def of %23, so the values are identical. Now suppose that (due to other code) one of %42 or %51 has a subrange that is L00000003. Trying to follow the copy chain for either %42 or %51 in L00000003 ends up at 100r for sub0 or 110r for sub1. The old code before %https://reviews.llvm.org/D49535 arbitrarily chose whichever one it found first in the subranges, which is bad.

With https://reviews.llvm.org/D49535, as fixed by https://reviews.llvm.org/D51848, it wants to find the same def for each subrange of %23 that contributes to L00000003, and it fails because it finds 100r and 110r.

Now, this probably could be fixed by something like followCopyChain returning what lanes it found a def in, and then valuesIdentical spotting that it didn't get all the lanes it wanted and retrying with the remaining ones, and coping with that being different.

But I still believe that trying to spot the identical case in subranges is pointless because we have already proved that the values are identical in main ranges. It is impossible for the values to be not identical in subranges; it's just a bit tricky to fix up the code to demonstrate that.

So (if one accepts that argument) I guess the two options are:

1. land this anyway instead of https://reviews.llvm.org/D51848, even though there is currently no test that fails on https://reviews.llvm.org/D51848 but passes with this fix (we still have the test from https://reviews.llvm.org/D51848 that fails if you have neither fix); or

2. land https://reviews.llvm.org/D51848 for now, and revisit this fix if I find a test that still fails for this reason later on.

What do you think?


Repository:
  rL LLVM

https://reviews.llvm.org/D52061