[PATCH] D36224: [TwoAddressInstructionPass] Replace subregister uses when processing tied operands

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

In https://reviews.llvm.org/D36224#1245944, @arsenm wrote:

> In https://reviews.llvm.org/D36224#1243458, @bjope wrote:
>
> > In https://reviews.llvm.org/D36224#1243333, @arsenm wrote:
> >
> > > The tests may pass, but is the output exactly the same? I think this should break the test in the listed example. There can't be a copy in the case it's trying to fix
> >
> >
> > Sorry, I don't think I understand what you mean. Are you talking about the example in the summary, or the added mir-test?
>
>
> The tests I committed with that revision


Oh, I see, you answered the question that was 13 months old :-)

Well, your old fix from here https://reviews.llvm.org/rL279804 touched two test cases:

  test/CodeGen/AMDGPU/indirect-addressing-si.ll
  test/CodeGen/AMDGPU/indirect-addressing-si-noopt.ll

I've been running all RUN-lines from both of those test cases, with -print-after-all, and compared the result on trunk with and without this patch. The result is identical!
So maybe there have been some other fix that has solved the same problem you fixed in https://reviews.llvm.org/rL279804 in some other way (or something that hides it).

We have had the patch from this review (revert of https://reviews.llvm.org/rL279804) active in our out-of-tree target ever since https://reviews.llvm.org/rL279804 started to break our testing.
And I think that given the test case added in this review it is quite obvious that the code on trunk is doing the wrong thing right now. This patch seems to fix the problem.

I'm actually not sure if this should impact the update of liveness info. LV is deprecated and should not be used after this pass (so I don't know if it really need to be updated). And everything about LIS here is unused code unless using -early-live-intervals. And we do not have any tests for that (and when I tried -early-live-intervals I got lots of other failures). Anyway, since https://reviews.llvm.org/rL279804 did not change anything about liveness info, I guess a revert of https://reviews.llvm.org/rL279804 should not impact it either.


Repository:
  rL LLVM

https://reviews.llvm.org/D36224