[PATCH] D135434: Propagate tied operands when copying a MachineInstr.

Tue Oct 11 02:37:37 PDT 2022

dmgreen added a comment.

In D135434#3846427 <https://reviews.llvm.org/D135434#3846427>, @simon_tatham wrote:

> In D135434#3845431 <https://reviews.llvm.org/D135434#3845431>, @dmgreen wrote:
>
>> This sounds OK to me, and is probably how I would fix this. But it will effect more than just the MachinePipeliner, anything that goes through CloneMachineInstr. Looking at where that is uses, that seems like most the uses should be OK. I'm don't think many backends will use these implicit tied uses, and copying them sounds more correct during a clone.
>
> Yes, I think that specifically because this is in the //copy// constructor, it seems to me that the expectation is that the output MachineInstr looks semantically identical to the input one.
>
>> Can this be removed now? I don't see any pipelining + inline assembly tests in the test suite.
>
> I agree that it //looks// as if that code ought to be redundant now. But I tried removing that code //and// my fix, and the only test that failed was my own new one. So I also agree that we don't seem to have any tests of it, unfortunately!

I agree that this is an improvement for the copy constructor. I worry that that if that code is left is and there is a case where the inlineasm is copied, we end up trying to copy the tied-operands twice and end up crashing. It seems that the pipeliner is OK with inline-asm in general, so it's probably worth making a test case if we can.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135434