[PATCH] D78010: [CodeGen] Add new function unionImplicitOps() to union implicit register

[Zhang Kang via Phabricator via llvm-commits]

ZhangKang marked 4 inline comments as done.
ZhangKang added a comment.

In D78010#1980249 <https://reviews.llvm.org/D78010#1980249>, @nemanjai wrote:

> I think that the question that needs to be answered is whether the duplicated implicit operands were intended or if they were just an oversight. If they were intended, we should understand that intention and see if it still makes sense and if anything relies on it. If it was just an oversight, we should see if anything relies on it or whether we are free to change the semantics.
>  In any case, if we do need to keep the existing semantics, I would much rather have separate functions to achieve the different semantics. Perhaps `copyImplicitOperands()` should remain as-is and we should add something like `unionImplicitOperands()` that would create the union that you describe here.


I have create a new function `unionImplicitOperands()`, it will skip those same implicit register and not support the RegMask operand.



================
Comment at: llvm/lib/CodeGen/MachineInstr.cpp:1425
+    if ((MO.isReg() && MO.isImplicit()) || MO.isRegMask()) {
+      if (SkipDuplicated && (findRegisterUseOperandIdx(MO.getReg()) != -1 ||
+                             findRegisterDefOperandIdx(MO.getReg()) != -1))
----------------
arsenm wrote:
> nemanjai wrote:
> > Won't this throw assertions on regmasks? And what should the behaviour actually be on multiple regmasks?
> This O(N^2) loop over operands isn't great
I will use a set to let the time complexity of finding same register to O(logN).


================
Comment at: llvm/lib/CodeGen/MachineInstr.cpp:1425
+    if ((MO.isReg() && MO.isImplicit()) || MO.isRegMask()) {
+      if (SkipDuplicated && (findRegisterUseOperandIdx(MO.getReg()) != -1 ||
+                             findRegisterDefOperandIdx(MO.getReg()) != -1))
----------------
ZhangKang wrote:
> arsenm wrote:
> > nemanjai wrote:
> > > Won't this throw assertions on regmasks? And what should the behaviour actually be on multiple regmasks?
> > This O(N^2) loop over operands isn't great
> I will use a set to let the time complexity of finding same register to O(logN).
Yes, it should throw assertion for RegMask. Because if an instruction has RegMask operand, it said  some registers we can't use, it may use in callee. We can't simply copy RegMask or union RegMask from one instruction to another instruction


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78010