Page MenuHomePhabricator

[CodeGen] Add new function unionImplicitOps() to union implicit register
AbandonedPublic

Authored by ZhangKang on Apr 13 2020, 3:03 AM.

Details

Summary

The function MachineInstr::copyImplicitOps(MachineFunction &MF, const MachineInstr &MI) and MachineInstrBuilder::copyImplicitOps(const MachineInstr &OtherMI) will copy all implicit ops from Src_MI to Dst_MI.

When the Src_MI and Dst_MI has the same implicit ops, if we still call copyImplicitOps(), we will get some redundant implicit reg.

For example:
The Src_MI is:

BLR implicit $lr, implicit $rm, implicit killed $r3

The Dst_MI is:

BCLR $cr5lt, implicit $lr, implicit $rm

After calling Dst_MI.copyImplicitOps(Src_MI), we will get:

BCLR $cr5lt, implicit $lr, implicit $rm, implicit $lr, implicit $rm, implicit killed $r3

On the some targets like PowerPC, it's obvious that the second implicit $lr, implicit $rm is redundant, should be removed.
The right result should be:

BCLR $cr5lt, implicit $lr, implicit $rm, implicit killed $r3

The function copyImplicitOps() should skip those implicit operands which both Src_MI and Dst_MI have.

This patch is to add a new parameter SkipDuplicated for copyImplicitOps(), so we can choose whether we need skip those duplicated operands.

Diff Detail

Event Timeline

ZhangKang created this revision.Apr 13 2020, 3:03 AM

Passing-by remark: i feel like while the patch description really explains
what is happening in this patch, it does not mention at all
*why* all this is happening, why this shouldn't be happening, etc.

arsenm added a subscriber: arsenm.Apr 13 2020, 8:45 AM

I agree this is a workaround

ZhangKang updated this revision to Diff 257178.Apr 13 2020, 6:53 PM

Rebase and fix the format.

ZhangKang edited the summary of this revision. (Show Details)Apr 13 2020, 7:11 PM
ZhangKang added reviewers: arsenm, lebedev.ri.

Passing-by remark: i feel like while the patch description really explains
what is happening in this patch, it does not mention at all
*why* all this is happening, why this shouldn't be happening, etc.

Have updated the summary to show *why* all this is happening, why this shouldn't be happening.

ZhangKang edited the summary of this revision. (Show Details)Apr 13 2020, 7:21 PM

Passing-by remark: i feel like while the patch description really explains
what is happening in this patch, it does not mention at all
*why* all this is happening, why this shouldn't be happening, etc.

For now, copyImplicitOps will blindly copy all the implicit ops no matter if we already have that implicit op. My understanding is that, we won't have any benefit if there is duplicate implicit operands. As this is a helper function to copy the implicit operands, I would suggest that, don't add a new parameter, just don't do the copy if it has. Does it make sense ?

lkail added a reviewer: sunfish.
lkail added a subscriber: lkail.Apr 13 2020, 8:56 PM

For now, copyImplicitOps will blindly copy all the implicit ops no matter if we already have that implicit op. My understanding is that, we won't have any benefit if there is duplicate implicit operands. As this is a helper function to copy the implicit operands, I would suggest that, don't add a new parameter, just don't do the copy if it has. Does it make sense ?

I think it should apply to most register-based targets, I have no idea if it applies to stack-based targets, like webassembly.

Passing-by remark: i feel like while the patch description really explains
what is happening in this patch, it does not mention at all
*why* all this is happening, why this shouldn't be happening, etc.

For now, copyImplicitOps will blindly copy all the implicit ops no matter if we already have that implicit op. My understanding is that, we won't have any benefit if there is duplicate implicit operands. As this is a helper function to copy the implicit operands, I would suggest that, don't add a new parameter, just don't do the copy if it has. Does it make sense ?

That would make more sense to me, yes.

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.

llvm/lib/CodeGen/MachineInstr.cpp
1430

Won't this throw assertions on regmasks? And what should the behaviour actually be on multiple regmasks?

arsenm added inline comments.Apr 14 2020, 7:16 AM
llvm/lib/CodeGen/MachineInstr.cpp
1430

This O(N^2) loop over operands isn't great

ZhangKang marked 4 inline comments as done.Apr 20 2020, 10:57 PM

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.

llvm/lib/CodeGen/MachineInstr.cpp
1430

I will use a set to let the time complexity of finding same register to O(logN).

1430

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

ZhangKang marked an inline comment as done.
ZhangKang retitled this revision from [CodeGen] Add a new parameter SkipDuplicated for copyImplicitOps() to [CodeGen] Add new function unionImplicitOps() to union implicit register.

add new function unionImplicitOps()

This comment was removed by lkail.
lkail added inline comments.Apr 22 2020, 8:08 PM
llvm/lib/CodeGen/MachineInstr.cpp
1446

Shall we just put this assertion after where MO is defined?

1455

Same as above.

ZhangKang updated this revision to Diff 259840.Apr 24 2020, 2:51 AM
ZhangKang marked an inline comment as done.

Move the assert before if condition.

steven.zhang added inline comments.Apr 27 2020, 1:03 AM
llvm/lib/CodeGen/MachineInstr.cpp
1440

Can we use the for range ?

ZhangKang updated this revision to Diff 260531.Apr 27 2020, 7:20 PM

Using range-for to instead for-loop.

ZhangKang marked 2 inline comments as done.Apr 27 2020, 7:20 PM
ZhangKang added inline comments.
llvm/lib/CodeGen/MachineInstr.cpp
1440

Done.

steven.zhang added inline comments.Apr 28 2020, 1:58 AM
llvm/lib/CodeGen/MachineInstr.cpp
1443

So, we don't need the isImplicit check any more.

ZhangKang updated this revision to Diff 260621.Apr 28 2020, 6:42 AM
ZhangKang marked an inline comment as done.

Remove the isImplicit().

ZhangKang marked 2 inline comments as done.Apr 28 2020, 6:44 AM
ZhangKang added inline comments.
llvm/lib/CodeGen/MachineInstr.cpp
1443

Done.

arsenm added inline comments.Apr 28 2020, 7:04 AM
llvm/lib/Target/PowerPC/PPCEarlyReturn.cpp
80–81 ↗(On Diff #260621)

Do you just want to setDesc here instead? It seems to me you expect the same implicit operands before and after in the test, and just want to change the opcode

lkail added inline comments.Apr 28 2020, 9:27 AM
llvm/test/CodeGen/PowerPC/early-ret.mir
112 ↗(On Diff #260621)

Is there any instruction define $v2 between the last instruction of bb.0.entry and current one missing from check lines?

ZhangKang marked 4 inline comments as done.Apr 28 2020, 8:10 PM
ZhangKang added inline comments.
llvm/lib/Target/PowerPC/PPCEarlyReturn.cpp
80–81 ↗(On Diff #260621)

In fact, I have another patch https://reviews.llvm.org/D76042 to use setDesc(). I have set the patch D76042 be Change Planed.
In fact, if we use setDesc(), it means that we have assumed that the old instruction's implicit operands are same with the new instruction's.

If this patch can be approved, I will abandoned the patch D76042. I think use unionImplicitOps is better, it doesn't assume that the old instruction's implicit operands are same with the new instruction's.

llvm/test/CodeGen/PowerPC/early-ret.mir
112 ↗(On Diff #260621)

No, no instruction is missing.

steven.zhang added inline comments.Apr 28 2020, 9:05 PM
llvm/lib/CodeGen/MachineInstr.cpp
1441

The assertion here is not needed. What we really need to care about is the regmask in the MI that we want to union. In fact, it is not hard to handle the case with regmask. I am ok to leave it as limitation.

ZhangKang marked 2 inline comments as done.

Remove the assert for src regmask operand.

arsenm added inline comments.Apr 29 2020, 8:18 AM
llvm/lib/Target/PowerPC/PPCEarlyReturn.cpp
80–81 ↗(On Diff #260621)

There's no reason to not rely on this assumption though. I can imagine a use for unionImplicitOps, but not for contexts such as this

ZhangKang marked 2 inline comments as done.May 11 2020, 2:59 AM
ZhangKang added inline comments.
llvm/lib/Target/PowerPC/PPCEarlyReturn.cpp
80–81 ↗(On Diff #260621)

I have removed calling unionImplicitOps() in PPCEarlyReturn.cpp here, and enabled the patch D76042 to use setDesc() to fix the bug in PPCEarlyReturn.cpp as you suggested.
Now there is no scene to call unionImplicitOps().

ZhangKang updated this revision to Diff 263130.May 11 2020, 3:00 AM
ZhangKang marked an inline comment as done.

Remove call unionImplicitOps() in PPCEarlyReturn.cpp.

@arsenm , I have removed calling unionImplicitOps() in PPCEarlyReturn.cpp.
Now there is no scene to call unionImplicitOps().
Do you think I should abandon this patch?

@arsenm , I have removed calling unionImplicitOps() in PPCEarlyReturn.cpp.
Now there is no scene to call unionImplicitOps().
Do you think I should abandon this patch?

Yes, unless there's a concrete use case (I can maybe imagine one, but I wouldn't encourage use of this without a better idea)

arsenm resigned from this revision.Jun 2 2020, 12:56 PM

No concrete use case, abandoned it.

ZhangKang abandoned this revision.Jun 3 2020, 2:33 AM