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

[Simon Tatham via Phabricator via llvm-commits]

simon_tatham added a comment.

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!

In D135434#3845660 <https://reviews.llvm.org/D135434#3845660>, @arsenm wrote:

> Is there a missing verifier check?

That sounds very plausible. I did try `-verify-machineinstrs` on the example that originally failed for me, and was a bit surprised that it reported no problems and still generated incorrect output code. So yes, it might well be that someone who knows the MachineVerifier might be able to add a check for ties based on checking the MachineInstr against the MCID. But it's large and has many confusing subfunctions and I'm not familiar enough with it to be able to dash that off quickly, so I hope it need not block fixing the actual correctness issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135434