This is an archive of the discontinued LLVM Phabricator instance.

Differential D57314

[PowerPC] more opportunity for converting reg+reg to reg+imm
ClosedPublic

Authored by shchenz on Jan 27 2019, 8:08 PM.

Details

Reviewers
nemanjai
jsji
hfinkel
steven.zhang
stefanp
Commits
rL352583: [PowerPC] more opportunity for converting reg+reg to reg+imm
Summary

PowerPC should convert following reg+reg to reg+imm.

body: |
  bb.0.entry:
    liveins: $x3, $f1, $x5  
    $x3 = ADDI8 $x5, 100  
    STFSX killed $f1, $zero8, $x3  
    STD killed $x3, killed $x5, 100  
    BLR8 implicit $lr8, implicit $rm

Fix the bug to make it work.

Diff Detail

Event Timeline

shchenz created this revision.Jan 27 2019, 8:08 PM
Herald added a subscriber: hiraditya. · View Herald TranscriptJan 27 2019, 8:08 PM
shchenz edited reviewers, added: steven.zhang; removed: • qshanz.Jan 27 2019, 8:09 PM
stefanp added a subscriber: stefanp.Jan 28 2019, 10:15 AM

Looked through this and tried a couple of things but overall it looks good.
My only comments are a couple of minor nit items.

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
3209

nit:
Since you have updated the condition in the code can you also update the comment above it?

llvm/test/CodeGen/PowerPC/convert-rr-to-ri-instr-add.mir
3

nit:
You should probably add to the test a comment to say what you are testing here.
It's not really obvious from the test because you just ask to stop after the peephole opt and that could mean you are looking for any number of things.

shchenz updated this revision to Diff 184002.Jan 28 2019, 5:41 PM

Thanks for your comment @stefanp. Updated.

shchenz added a reviewer: stefanp.Jan 28 2019, 5:41 PM
nemanjai accepted this revision.Jan 29 2019, 5:14 AM

Oh yeah, the existing condition was always true. Thanks for fixing this. LGTM.

This revision is now accepted and ready to land.Jan 29 2019, 5:14 AM
shchenz updated this revision to Diff 184213.Jan 29 2019, 5:50 PM

@nemanjai Thanks for your comments.

Change test comment position to avoid YAML grammar error.

shchenz updated this revision to Diff 184215.Jan 29 2019, 5:55 PM

use '#' as a comment marker outside of body.

Closed by commit rL352583: [PowerPC] more opportunity for converting reg+reg to reg+imm (authored by shchenz). · Explain WhyJan 29 2019, 5:59 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
llvm/
lib/
Target/
PowerPC/
4 lines
test/
CodeGen/
PowerPC/
17 lines

Diff 184213

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp

Show First 20 Lines • Show All 3,199 Lines • ▼ Show 20 Lines for (; It != E; ++It) {
if (It->modifiesRegister(Reg, &getRegisterInfo()) && (&*It) != &DefMI) if (It->modifiesRegister(Reg, &getRegisterInfo()) && (&*It) != &DefMI)
return false; return false;
// Made it to DefMI without encountering a clobber. // Made it to DefMI without encountering a clobber.
if ((&*It) == &DefMI) if ((&*It) == &DefMI)
break; break;
} }
assert((&*It) == &DefMI && "DefMI is missing"); assert((&*It) == &DefMI && "DefMI is missing");
// If DefMI also uses the register to be forwarded, we can only forward it // If DefMI also defines the register to be forwarded, we can only forward it
// if DefMI is being erased. // if DefMI is being erased.
stefanpUnsubmitted
Not Done
ReplyInline Actions

nit:
Since you have updated the condition in the code can you also update the comment above it?

stefanp: nit: Since you have updated the condition in the code can you also update the comment above it?
if (DefMI.readsRegister(Reg, &getRegisterInfo())) if (DefMI.modifiesRegister(Reg, &getRegisterInfo()))
return KillDefMI; return KillDefMI;
return true; return true;
} }
bool PPCInstrInfo::isImmElgibleForForwarding(const MachineOperand &ImmMO, bool PPCInstrInfo::isImmElgibleForForwarding(const MachineOperand &ImmMO,
const MachineInstr &DefMI, const MachineInstr &DefMI,
const ImmInstrInfo &III, const ImmInstrInfo &III,
▲ Show 20 Lines • Show All 567 Lines • Show Last 20 Lines

llvm/test/CodeGen/PowerPC/convert-rr-to-ri-instr-add.mir

  • This file was added.
# RUN: llc -mtriple=powerpc64le--linux-gnu -stop-after ppc-pre-emit-peephole %s -o - -verify-machineinstrs | FileCheck %s
---
stefanpUnsubmitted
Not Done
ReplyInline Actions

nit:
You should probably add to the test a comment to say what you are testing here.
It's not really obvious from the test because you just ask to stop after the peephole opt and that could mean you are looking for any number of things.

stefanp: nit: You should probably add to the test a comment to say what you are testing here. It's not…
name: testFwdOperandKilledAfter
# CHECK: name: testFwdOperandKilledAfter
tracksRegLiveness: true
body: |
bb.0.entry:
liveins: $x3, $f1, $x5
; ADDI8 + STFSX can be converted to ADDI8 + STFS even ADDI8 can not be erased.
$x3 = ADDI8 $x5, 100
STFSX killed $f1, $zero8, $x3
; CHECK: STFS killed $f1, 100, $x5
STD killed $x3, killed $x5, 100
; CHECK: STD killed $x3, killed $x5, 100
BLR8 implicit $lr8, implicit $rm
...