This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Modify the hasSideEffects of MTLR and MFLR from 1 to 0
ClosedPublic

Authored by ZhangKang on Dec 11 2019, 9:55 PM.

Details

Summary

If we didn't set the value for hasSideEffects bit in our td file, llvm-tblgen will set it as true for those instructions which has no match pattern.
The instructions MTLR and MFLR don't set the hasSideEffects flag and don't have match pattern, so their hasSideEffects flag will be set true by llvm-tblgen.
But in fact, we can use [LR] to model the two instructions, so they should not have SideEffects.

This patch is to modify the hasSideEffects of MTLR and MFLR from 1 to 0.

Diff Detail

Event Timeline

ZhangKang created this revision.Dec 11 2019, 9:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 11 2019, 9:55 PM
ZhangKang added reviewers: Restricted Project, hfinkel.Dec 11 2019, 9:57 PM
ZhangKang added subscribers: power-llvm-team, echristo.
ZhangKang edited the summary of this revision. (Show Details)Dec 11 2019, 10:18 PM
lkail added a subscriber: lkail.Dec 11 2019, 11:05 PM
This comment was removed by lkail.
jsji requested changes to this revision.Dec 12 2019, 10:20 AM
jsji added a subscriber: steven.zhang.

@steven.zhang can you work with @ZhangKang to see why, and whether we rely on hasSideEffects for a better schedule right now. Thanks.

llvm/test/CodeGen/PowerPC/CSR-fit.ll
32

I see this is actually causing degradation here. mtlr is 5 cycle instruction, moving them together is not a good idea.

This revision now requires changes to proceed.Dec 12 2019, 10:20 AM
steven.zhang added inline comments.Dec 13 2019, 12:16 AM
llvm/test/CodeGen/PowerPC/CSR-fit.ll
32

It is trying to move it away from the dependency of "ld r0, 16(r1)" which is 4 cycle. The critical path of the region is 9 cycles(4 cycles(ld) + 5 cycles(mtlr)), and the two loads here won't matter too much. So, the scheduling works fine from my view.

jsji added a comment.Dec 13 2019, 7:10 AM

@ZhangKang Can we get some performance data for this change. Thanks.

llvm/test/CodeGen/PowerPC/ppcf128-constrained-fp-intrinsics.ll
1383

This is way to far from ld 0,16(1) now.

Seems currently we haven't modeled mtlr correctly. In PPCFrameLowering

if (MustSaveLR)
  BuildMI(MBB, StackUpdateLoc, dl, MTLRInst).addReg(ScratchReg);

We might lack a implicit-def of $lr here. As a result, we might get wrong code if set hasSideEffect = 0 for mtlr. What do you think?

I have do the test, if we use Uses = [LR8] like MFLR, we will get M$x0 = MFLR8 implicit $lr8. And if we use Defs = [LR8] like MTLR, we will get MTLR8 $x0, implicit-def $lr8. So the code you pointed out is right.
In fact, those instructions which use Uses=[ ] or Defs = [ ] in td files will be added implicit register automatically in MIR.

@ZhangKang Can we get some performance data for this change. Thanks.

@jsji , Ok, I will do it soon.

steven.zhang added inline comments.Dec 15 2019, 6:42 PM
llvm/test/CodeGen/PowerPC/ppcf128-constrained-fp-intrinsics.ll
1383

But those load in between didn't have any dependency. So, it is likely an heuristic between the resource and latency.

Jim added a subscriber: Jim.Dec 15 2019, 7:24 PM

If an instruction has side effects, it would be a scheduling barrier.
Not any instruction can be scheduled crossing it.

llvm/lib/Target/PowerPC/PPCInstr64Bit.td
412

Blank line shouldn't deleted here.

Fix the empty line.

ZhangKang marked 2 inline comments as done.Dec 15 2019, 10:25 PM
ZhangKang added inline comments.
llvm/lib/Target/PowerPC/PPCInstr64Bit.td
412

Done.

Something doesn't match up here. The description/title say that you are changing the flag for both MFLR/MTLR. But in the code I only see the change for MTLR. Why is that?
Also, the context is missing from this patch so it cannot be reviewed properly, please fix that.

ZhangKang updated this revision to Diff 234431.Dec 17 2019, 5:30 PM
ZhangKang marked an inline comment as done.

Add the context.

Something doesn't match up here. The description/title say that you are changing the flag for both MFLR/MTLR. But in the code I only see the change for MTLR. Why is that?
Also, the context is missing from this patch so it cannot be reviewed properly, please fix that.

Have updated the patch to add the context.

I have run the performance for this patch, there is no regression.

ZhangKang removed a reviewer: jsji.Dec 24 2019, 6:21 PM
ZhangKang removed a reviewer: Restricted Project.
ZhangKang added reviewers: Restricted Project, jsji.
jsji accepted this revision as: jsji.Dec 24 2019, 6:50 PM

LGTM.

This revision is now accepted and ready to land.Dec 24 2019, 6:50 PM
This revision was automatically updated to reflect the committed changes.