This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Fix RM operands for some instructions
ClosedPublic

Authored by ZhangKang on Jun 7 2020, 9:03 PM.

Details

Summary

Some instructions have set the wrong [RM] flag, this patch is to fix it.

Instructions x(v|s)r(d|s)pi[zmp]? and fri[npzm] use fixed rounding directions without referencing current rounding mode.
This patch will remove their use to RM in instructions definition.

Also, the SETRNDi, SETRND, BCLRn, MTFSFI, MTFSB0, MTFSB1, MTFSFb, MTFSFI, MTFSFI_rec, MTFSF, MTFSF_rec should also fix the RM flag.

Diff Detail

Event Timeline

ZhangKang created this revision.Jun 7 2020, 9:03 PM
ZhangKang updated this revision to Diff 269132.Jun 8 2020, 2:15 AM

Add a test case.

ZhangKang updated this revision to Diff 270056.Jun 11 2020, 1:24 AM

Fix XSRDPI/XSRDPIM/XSRDPIP/XSRDPIZ, XVRDPI/XVRDPIM XVRDPIP XSRDPIZ, XVRSPI/XVRSPIM XVRSPIP XSRSPIZ

jsji requested changes to this revision.Jul 17 2020, 11:41 AM

Please rebase. Also why this depends on D76042?

This revision now requires changes to proceed.Jul 17 2020, 11:41 AM

Also why this depends on D76042?

The patch D76042 will remove the redundant the implicit operands in ppc-early-ret pass.
Below code is from the origin case test/CodeGen/PowerPC/early-ret.mir.

109   ; CHECK:   BCLR killed renamable $cr0eq, implicit $lr, implicit $rm, implicit $lr, implicit $rm, implicit killed $v2
110   ; CHECK: bb.1.entry:
111   ; CHECK:   renamable $cr0 = FCMPUS killed renamable $f1, killed renamable $f2
112   ; CHECK:   BCLRn killed renamable $cr0eq, implicit $lr, implicit $rm, implicit killed $v2

The patch D76042 will fix the redundant implicit in line 109, you can see the line 112 is right. In fact, here line 112 is right,
because we have forgotten to set 'Uses = [LR, RM]', this is what this patch does. So I set this patch depends on D76042, or we will
see the line 112 will be wrong after fix the 'RM' flag.

1526     let isReturn = 1, Uses = [LR, RM] in
1527     def BCLR  : XLForm_2_br2<19, 16, 12, 0, (outs), (ins crbitrc:$bi),
1528                              "bclr 12, $bi, 0", IIC_BrB, []>;
1529     def BCLRn : XLForm_2_br2<19, 16, 4, 0, (outs), (ins crbitrc:$bi),
1530                              "bclr 4, $bi, 0", IIC_BrB, []>;
ZhangKang removed a reviewer: jsji.Jul 19 2020, 9:11 AM
ZhangKang removed a reviewer: Restricted Project.
ZhangKang added reviewers: jsji, Restricted Project.
jsji added a subscriber: qiucf.Jul 19 2020, 7:56 PM

Looks like some duplicate work in D83471, can you please sync with @qiucf , and either split the statics out or merge two patches.

qiucf added a comment.Jul 19 2020, 8:01 PM

Looks like some duplicate work in D83471, can you please sync with @qiucf , and either split the statics out or merge two patches.

Oh, I didn't see this patch already contains these changes.. I'd like to abandon D83471. @ZhangKang we can also remove ref to RM for fri[npzm]. Thanks.

ZhangKang edited the summary of this revision. (Show Details)Jul 19 2020, 10:51 PM
jsji added a comment.EditedJul 20 2020, 6:56 AM

Please add more tests for affected opcodes, not just SETRND. Thanks.

ZhangKang updated this revision to Diff 279253.Jul 20 2020, 8:09 AM

Add the case for MTFSF.

ZhangKang updated this revision to Diff 279273.Jul 20 2020, 9:18 AM

Add more cases.

jsji added a comment.Jul 23 2020, 1:53 PM

Please also add tests for mtfsb0 etc.

llvm/lib/Target/PowerPC/PPCInstrInfo.td
1511

int_ppc_setrnd will read FPSCR which include RM, why we want to remove the use of it?

2917–2918

mtfsb0 will only change RM when the bit is 30/31, which is actually in SETRNDi, SETRND.. So I think we should not always Def RM here? Maybe some PatLeaf to check bits and only set it with 30/31 bits?
Similar for mtfsfi, mtfsf?

ZhangKang marked 2 inline comments as done.Jul 28 2020, 9:31 AM
llvm/lib/Target/PowerPC/PPCInstrInfo.td
1511

Yes, I should not remove the use for RM here, because we will use MFFS to store the FPSCR.

2917–2918

Has removed the rm for mtfsb0 mtfsb1 and mtfsfi.

ZhangKang updated this revision to Diff 281260.Jul 28 2020, 9:36 AM
ZhangKang marked 2 inline comments as done.

Update the patch to follow reviewers' comments.

ZhangKang updated this revision to Diff 281264.Jul 28 2020, 9:43 AM

FIx the comments.

jsji accepted this revision as: jsji.Jul 28 2020, 11:43 AM

LGTM. Thanks.

llvm/lib/Target/PowerPC/PPCInstrInfo.td
4328

Can we split this into another patch, this is not related to RM.

This revision is now accepted and ready to land.Jul 28 2020, 11:43 AM
jsji added inline comments.Jul 28 2020, 11:45 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
11989

This will read RM as well, so we should add implicit operand here too.

ZhangKang updated this revision to Diff 281444.Jul 28 2020, 6:30 PM
ZhangKang marked 2 inline comments as done.

Remove the CR1

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
11989

I only remove use/def RM for MTFSB0/MTFSB1/MTFSFI, because they use the constant, we can know whether they will use the RM.
For those instructions which use the Reg to set FPSCR, I have set it use/def RM in the td files. So MFFS will be added RM automatically.

llvm/lib/Target/PowerPC/PPCInstrInfo.td
4328

OK, I will commit a NFC patch directly to fix the CR1.

This revision was automatically updated to reflect the committed changes.