[PATCH] D107582: [CodeGen] Fix dependence breaking for tied operands

Mon Oct 18 10:18:21 PDT 2021

danilaml added a comment.

In D107582#3070678 <https://reviews.llvm.org/D107582#3070678>, @MatzeB wrote:

>> @MatzeB Do you have any thoughts on this?
>
> Unfortunately this pass has a very unconventional way to analyze things and manage liveness. It's hard to understand what is going on, so it's hard to judge changes to this pass...

I agree ant that's what I tried to express in my summary. I don't believe this patch solves all potential issues I've encountered, but as you said, the way the Pass tracks it's state is very convoluted so I was hesitant to make any more involved changes (I even had trouble reconstructing the context of the patch after combing back to it right now).

> It seems the idea of this patch is that check for tied operands in line 225/226 (in the original) is performed in a scan separate from the other operands? I don't see why this improves things right now, but I also don't see the harm. @danilaml Can you explain why the change helps?

The overall issue here is that this pass doesn't seem to correctly handle instructions with implicit defs and tied reg operands. One case of it was fixed in https://reviews.llvm.org/D4351 , and this tries to fix similar issue (see the end of the summary and the test case), but basically it ends up with
`$esi = XOR32rr undef $ecx, undef $ecx, implicit-def dead $eflags, implicit-def $rsi`
in place of
`$esi = XOR32rr undef $esi, undef $esi, implicit-def dead $eflags, implicit-def $rsi`

which is illegal (no asserts but codegen randomly converges it to "legal" state) and can lead to the bad codegen.

> If this does indeed only make the pass more conservative in some cases with tied operands, then I'm fine landing it.

I think what my change does is effectively prohibit dep breaking for regs in tied operands if they are also used in implicit def in the same instruction (the case for most 32-bit wide MIs on x64).
Previously, the code would encounter the check before it processed all MOs, that might've changed the Classes[Reg] to `-1`. I'm still not sure why the original fix equates `-1` state with being live (which it is not).

>> On x64 32-bit instructions are often (although inconsistently) annotated with implicit-def of the 64-bit reg alias.
>
> The idea is that this depends on the user of the instruction. For example if there is a use reading the whole %rsi register and the defining instruction only writes to %esi then the instruction should be annotated with the implicit-def, there's no need for it if the uses only read %esi so you can see both variants in practice.

I see, that makes sense.

>> It doesn't look like this pass handles implicit defs at all (sans, possibly, near MI.getDesc().getNumOperands()).
>> I don't think it's safe to assume that one can freely change implicit-def of the instruction, so perhaps adding all such encountered regs to KeepRegs could be a solution?
>
> Generally I think you can change registers for operands marked with the `renamable` attribute, but must not touch other operands. With that said, this pass clearly doesn't care; it was written before `renamable` was introduced and was never updated to use it :-(

In the MIR I've encountered (and reduced the test case from) for x86 I didn't see the implicit-defs annotated that way. And as you said, this pass doesn't seem to care about it. When reading the code for this pass I had a lot of "how does it even work" moments, so I'm not prepared to attempt fixing these issues/pass (especially considering how hard it is to find someone to review this patch). All in all, I'm convinced that this pass would still be subtly broken and this patch would just make it slightly less so in at least one example (encountered in the wild).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107582