[PATCH] D126048: [SplitKit] Handle early clobber + tied to def correctly

[Kito Cheng via Phabricator via llvm-commits]

kito-cheng added inline comments.


================
Comment at: llvm/lib/CodeGen/SplitKit.cpp:1381
+        unsigned OpIdx = MI->getOperandNo(MOItr);
+        unsigned DefOpIdx = MI->findTiedOperandIdx(OpIdx);
+        const MachineOperand &DefOp = MI->getOperand(DefOpIdx);
----------------
arcbbb wrote:
> kito-cheng wrote:
> > arcbbb wrote:
> > > The logic here looks similar to LiveIntervalCalc::extendToUses that checks for early-clobber redefs.
> > > Can it be simplified with `OpIdx = (&MO - &MI->getOperand(0))` and `MI->isRegTiedToDefOperand(OpIdx, &DefOpIdx)`?
> > `MI->getOperandNo(MOItr)` did almost similar thing[1] as `(&MO - &MI->getOperand(0))`, but having better readability.
> > 
> > And `isRegTiedToDefOperand` called `findTiedOperandIdx` in the function implementation[2], so I am not sure it's a simplification?
> > 
> > [1] https://llvm.org/doxygen/MachineInstr_8h_source.html#l00685
> > [2] https://llvm.org/doxygen/MachineInstr_8h_source.html#l00685
> I think it can save the `for` loop and do checks for you
> so the code would look like
> ```lang=diff
> -    } else
> -      Idx = Idx.getRegSlot(true);
> +    } else {
> +      unsigned OpNo = MI->getOperandNo(&MO);
> +      unsigned DefIdx;
> +      bool isEarlyClobber = false;
> +      if (MI->isRegTiedToDefOperand(OpNo, &DefIdx))
> +        isEarlyClobber = MI->getOperand(DefIdx).isEarlyClobber();
> +      Idx = Idx.getRegSlot(isEarlyClobber);
> +    }
> ```
Thanks, save the loop seems great!


================
Comment at: llvm/lib/CodeGen/SplitKit.cpp:1376
+      // in [16e, 32d).
+      unsigned OpIdx = MI->findRegisterUseOperandIdx(MO.getReg());
+      unsigned DefOpIdx = MI->findTiedOperandIdx(OpIdx);
----------------
MatzeB wrote:
> kito-cheng wrote:
> > MatzeB wrote:
> > > Use `getOperandNo` that's faster and avoids trouble if multiple operands use the same register.
> > Seems like `getOperandNo` only work with iterator? and that is what we don't have here :(
> > 
> > Anyway I change to that, but it is because I hit a regression for a testcase from X86...so I just try to traverse all input operand see which operand is tied-def and using same register as MO here...
> > 
> > See CodeGen/X86/statepoint-invoke-ra.mir:
> > ```
> >   dead %40:gr64 = STATEPOINT 2882400000, 0, 1, target-flags(x86-plt) @quux, $edi, 2, 0, 2, 0, 2, 6, 1, 4, %stack.0, 0, 1, 4, %stack.1, 0, 1, 4, %stack.2, 0, 1, 4, %stack.3, 0, %40:gr64, 1, 4, %stack.4, 0, 2, 1, %40:gr64(tied-def 0), 2, 0, 2, 1, 0, 0, <regmask $bh $bl $bp $bph $bpl $bx $ebp $ebx $hbp $hbx $rbp $rbx $r12 $r13 $r14 $r15 $r12b $r13b $r14b $r15b $r12bh $r13bh $r14bh $r15bh $r12d $r13d $r14d $r15d $r12w $r13w $r14w $r15w $r12wh and 3 more...>, implicit-def $rsp, implicit-def $ssp :: (volatile load store (s32) on %stack.0), (volatile load store (s32) on %stack.1), (volatile load store (s32) on %stack.2), (volatile load store (s32) on %stack.3), (volatile load store (s32) on %stack.4)
> > ```
> > 
> > %40 appeared 3 times in this instruction, one for def, and two for use, and only one def has `tied-def 0`.
> Well it's defined as `using const_mop_iterator = const MachineOperand *;`, so you should be good passing in the pointer, there's a some other places in the code that do the same.
Thanks, I didn't notice it's just an alias name of MachineOperand pointer :P


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126048