Page MenuHomePhabricator

[RISCV] remove redundant instruction when eliminate frame index
ClosedPublic

Authored by StephenFan on Dec 2 2020, 6:45 AM.

Details

Summary

The reason for generating mv a0, a0 instruction is when the stack object offset is large then int<12>. To deal this situation, in the elimintateFrameIndex function, it will
create a virtual register, which needs the register scavenger to scavenge it. If the machine instruction that contains the stack object and the opcode is ADDI(the addi
was generated by frameindexNode), and then this instruction's destination register was the same as the register that was generated by the register scavenger, then the
mv a0, a0 was generated. So to eliminnate this instruction, in the eliminateFrameIndex function, if the instrution opcode is ADDI, then the virtual register can't be created

Diff Detail

Event Timeline

StephenFan created this revision.Dec 2 2020, 6:45 AM
StephenFan requested review of this revision.Dec 2 2020, 6:45 AM
jrtc27 added a comment.Dec 2 2020, 6:56 AM

This is a bit ugly. Surely a simple peephole optimisation should be able to fix this and any other cases more generally? But also please give this a better title and commit message.

jrtc27 added a comment.Dec 2 2020, 7:06 AM

It's also wrong if the ADDI has FrameReg as its destination. A neater approach might be to always use the destination register (if there is one, using general instruction query information) as the destination for the ADD, keeping the scratch register for the movImm call. Then you'd only need special logic to work out whether you need to keep the (modified) existing instruction.

StephenFan added a reviewer: jrtc27.

It's also wrong if the ADDI has FrameReg as its destination. A neater approach might be to always use the destination register (if there is one, using general instruction query information) as the destination for the ADD, keeping the scratch register for the movImm call. Then you'd only need special logic to work out whether you need to keep the (modified) existing instruction.

I agree that it is wrong if the ADDI has FrameReg as its destination. And I adapted your suggestion.

StephenFan retitled this revision from [RISCV] remove instruction mv a0, a0 to [RISCV] remove redundant instruction when eliminate frame index.Dec 10 2020, 10:36 PM
kito-cheng added inline comments.Dec 10 2020, 10:54 PM
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
216

Just curios, does it possible get any other opcode than ADDI? below code are just update Offset, I guess that means we already assume it must be ADDI here? But I could be wrong since I didn't seriously tracing here before.

If my assumption is right then the code could be further simplified?

StephenFan added inline comments.Dec 11 2020, 4:52 AM
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
216

The ADDI is generated when the frameindexSDNode be selected to ADDI

case ISD::FrameIndex: {
  SDValue Imm = CurDAG->getTargetConstant(0, DL, XLenVT);
  int FI = cast<FrameIndexSDNode>(Node)->getIndex();
  SDValue TFI = CurDAG->getTargetFrameIndex(FI, VT);
  ReplaceNode(Node, CurDAG->getMachineNode(RISCV::ADDI, DL, VT, TFI, Imm));
  return;
}

In the eliminateFrameIndex function, I think the instructions that contains the frameindex operand are
ADDI (show above) and load/store instruction that come from loadRegFromStackSlot and storeRegToStackSlot function.

lenary resigned from this revision.Jan 14 2021, 9:42 AM
jrtc27 added inline comments.Jan 21 2021, 8:28 AM
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
216

If we had reg+reg addressing then we could fold the add into loads/stores, but given we don't I think this is the only case.

StephenFan added inline comments.Feb 5 2021, 5:36 AM
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
216

Do you talk about what RISCVDAGToDAGISel::doPeepholeLoadStoreADDI function do ?

asb accepted this revision.Feb 25 2021, 3:38 AM

This needs a rebase, but LGTM to land after that. Thanks!

This revision is now accepted and ready to land.Feb 25 2021, 3:38 AM