Page MenuHomePhabricator

[PowerPC] Ignore implicit register operands for MCInst
ClosedPublic

Authored by ZhangKang on Mar 31 2020, 12:25 AM.

Details

Summary

For below case:
cat bool-math.mir

---
name:            add_zext_cmp_mask_same_size_result
body:             |
  bb.0:
    liveins: $x3

    renamable $r3 = RLWINM killed renamable $r3, 0, 31, 31, implicit $x3
    renamable $r3 = SUBFIC killed renamable $r3, 27, implicit-def dead $carry, implicit-def $x3
    BLR8 implicit $lr8, implicit $rm, implicit killed $x3

...

Use below command to build it:

llc -start-after=ppc-early-ret bool-math.mir -o bool-math.s

Before this patch, we will get:

	rlwinm 3, 3, 0, 31, 31
	subfic 3, 3, 27
	blr

Note that rlwinm 3, 3, 0, 31, 31<--> clrlwi 3, 3, 31. But the mir case use implicit $x3 for RLWINM and PPC conserve the implicit $x3 for MCInst, so above case won't use Extended Mnemonic.


When doing the conversion: MachineInst -> MCInst, we should ignore the implicit operands.

I have seen ARM & AMDGPU have ignored the implicit operands when converting MachineInst -> MCInst.

ARM

ARMMCInstLower.cpp

72 bool ARMAsmPrinter::lowerOperand(const MachineOperand &MO,
73                                  MCOperand &MCOp) {
74   switch (MO.getType()) {
75   default: llvm_unreachable("unknown operand type");
76   case MachineOperand::MO_Register:
77     // Ignore all implicit register operands.
78     if (MO.isImplicit())
79       return false;
80     assert(!MO.getSubReg() &

AMDGPU

AMDGPUMCInstLower.cpp

126 bool AMDGPUMCInstLower::lowerOperand(const MachineOperand &MO,
127                                      MCOperand &MCOp) const {

170   case MachineOperand::MO_RegisterMask:
171     // Regmasks are like implicit defs.
172     return false;
173   }
174 }

After this patch, we will get

	clrlwi	3, 3, 31
	subfic 3, 3, 27
	blr

Diff Detail

Event Timeline

ZhangKang created this revision.Mar 31 2020, 12:25 AM

I think this is a good catch. InstAlias match the MI strictly including the implicit operands. We can either try to loose that check or remove the implicit operand when lowering the MI to MCInst, as it is not needed anymore. And please test this patch completely as this patch expose much more alias opportunities that didn't test before.

llvm/test/CodeGen/PowerPC/atomics-regression.ll
1234

Could you please explain more on the extra "#" here ?

llvm/test/CodeGen/PowerPC/xray-conditional-return.ll
42–43

I don't see this change relative with alias.

ZhangKang marked 4 inline comments as done.Mar 31 2020, 2:16 AM

I think this is a good catch. InstAlias match the MI strictly including the implicit operands. We can either try to loose that check or remove the implicit operand when lowering the MI to MCInst, as it is not needed anymore. And please test this patch completely as this patch expose much more alias opportunities that didn't test before.

Lit test and Lnt test has passed, and those modified assembly I have checked.

llvm/test/CodeGen/PowerPC/atomics-regression.ll
1234

This case is updated by update_llc_test_checks.py, last update is in Aug 30, 2019. After last update, someone modify the assembly output to add extra #.

llvm/test/CodeGen/PowerPC/xray-conditional-return.ll
42–43

In fact, the old assembly is:

cmpw 0, R1, R2
beq 0, Target

The new assembly is:

cmpw R1, R2
beq 0, Target

Because cmpw R1, R2 has omit CR0, I must specify beq use the CR0.

In fact, the beq 0, Target should also omit CR0 to beq Target, but it's not this patch should do.

jsji added inline comments.Mar 31 2020, 7:38 AM
llvm/test/CodeGen/PowerPC/atomics-regression.ll
1234

Can you please find out the patch? If the change is reasonable, then we should commit a NFC patch to update the testcases first, then rebase.

ZhangKang marked 4 inline comments as done.Mar 31 2020, 9:45 AM
ZhangKang added inline comments.
llvm/test/CodeGen/PowerPC/atomics-regression.ll
1234

This is caused by https://reviews.llvm.org/D63957 ,it will be fixed soon.

ZhangKang updated this revision to Diff 256228.Apr 9 2020, 3:21 AM
ZhangKang marked an inline comment as done.

Rebase and set the parent.

steven.zhang accepted this revision.EditedApr 9 2020, 6:38 PM

LGTM now. But pls wait for some days to see if Jinsong or someone else have more comments.

This revision is now accepted and ready to land.Apr 9 2020, 6:38 PM
This revision was automatically updated to reflect the committed changes.