[PATCH] D110829: [X86] Copy registers in reverse order in convertToThreeAddress

Fri Oct 1 09:34:07 PDT 2021

MatzeB added a comment.

On a general note, having the kill flag appear only on one operand seems sketchy to me:

> Note that in the ADD32rr, %0 was used twice and the first use had a kill flag, which is MachineInstr::addRegisterKilled does.

Glancing at `addRegisterKilled` seems to me like it would add the flag to all uses within an instruction (at least for vregs). There is a shortcut that aborts if it finds a pre-existing killflag where it wouldn't continue adding it to the other operands. But if that is the case then some other code beforehand got us into the odd situation of having the kill flag only on some operands.

I think ideally we would enforce that kill flag use is consistent within in instruction (either all uses of the same vreg use or do not use the kill flag). But it seems we are currently not so that would be work cleaning that up and enforcing it with the machine verifier. Anyway that seems out of the scope of this change, so for now we probably have to assume the kill flag is inconsistent between operands.

> I don't know if it's safe to rely on the kill flag being on the first operand that uses a particular reg. If not I'll rework the patch.

This seems accidental. I think ideally we would enforce consistency of the flag within an instruction. Short of that we better not assume anything like just the first operand having a kill flag...

> I don't know how to test this without removing the false from addPass(&TwoAddressInstructionPassID, false) in TargetPassConfig, which will currently still fail for a few other reasons.

Maybe using `llc -run-pass liveintervals,twiaddressinstruction` in a .mir test would work?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110829