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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

> @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...

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?

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

> 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.

> 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 :-(



================
Comment at: llvm/lib/CodeGen/CriticalAntiDepBreaker.cpp:224
+
+  for (unsigned i = 0, e = MI.getNumOperands(); i != e; ++i) {
+    MachineOperand &MO = MI.getOperand(i);
----------------
MatzeB wrote:
> 
Would it make sense to compute a `bool HasTiedOperand` variable in the first loop and skip the 2nd one if we didn't see any tied operands?


================
Comment at: llvm/lib/CodeGen/CriticalAntiDepBreaker.cpp:224-228
+  for (unsigned i = 0, e = MI.getNumOperands(); i != e; ++i) {
+    MachineOperand &MO = MI.getOperand(i);
+    if (!MO.isReg()) continue;
+    Register Reg = MO.getReg();
+    if (Reg == 0) continue;
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107582