Page MenuHomePhabricator

[RISCV][TableGen] Mark MachineInstr with FrameIndex as not compressible
ClosedPublic

Authored by piggynl on Jul 18 2022, 4:36 AM.

Details

Summary

If a MachineInstr's operand should be Reg in compiler's output but is currently FrameIndex, isCompressibleInst() will terminate at MachineOperandType::getReg().

This patch adds .isReg() checks to make isCompressibleInst() return false for these MachineInstr, allowing getInstSizeInBytes() to return a value and EstimateFunctionSizeInBytes() to work as intended.

See https://reviews.llvm.org/D129999#3694222 for details.

Diff Detail

Event Timeline

piggynl created this revision.Jul 18 2022, 4:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2022, 4:36 AM
piggynl requested review of this revision.Jul 18 2022, 4:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2022, 4:36 AM
piggynl updated this revision to Diff 445488.Jul 18 2022, 7:27 AM

Update test.

Ping for kindly review :)

This seems to make sense but can you please provide more information about the scenario where this is called and the instruction still has a FrameIndex? I understand you want to run this earlier but more information might be useful.

Being conservative about the size seems correct (e.g. for the branch relaxation pass we want to be conservative), but if anyone has any info to the contrary please indicate.

piggynl added a comment.EditedAug 2 2022, 11:32 AM

Sure! I'll explain the details about why I want to make this change.

In RISCV, j offset is used to perform unconditional branches with a smaller-than-1MiB offset. However, for branches larger than 1MiB, where the offset is outside the bounds of a 20-bit signed integer, we must use jump offset, rt (which assembler will expand to an auipc followed by a jr).

In BranchRelaxation pass, we find out-of-range offset branches and replace them with indirect branches (which is jump in RISCV). Since BranchRelaxation runs much later than register allocation and PEI, and compared with j, jump requires an additional scratch register rt, we use a RegisterScavenger to find an available register as the scratch register. https://reviews.llvm.org/D130560 handles the case where there is no scavenged register. We spill a register to the stack and use that register as the scratch register if there is no scavenged register. To reach that, we have to reserve an emergency spill slot in the stack.

PEI pass first calls TargetFrameLowering::processFunctionBeforeFrameFinalized(). Then PEI::replaceFrameIndices() replaces all FrameIndex MachineOperand with physical register references and actual offsets. We add the spill slot in processFunctionBeforeFrameFinalized() if we estimate the function has a large size (which means it is possible that have an out-of-range branch). We iterate all MachineInstr in the MachineFunction, calls TargetInstrInfo::getInstSizeInBytes(), and use the sum of the results as the estimated function size. Due to the fact that this step is run prior to replaceFrameIndices(), FrameIndex MachineOperand may be present in MachineInstr when determining the size of MachineInstr. Without this patch, if a MachineInstr's operand should be Reg in compiler's output but is currently FrameIndex, isCompressibleInst() will terminate at MachineOperandType::getReg().

I added the .isReg() checks in this patch to make isCompressibleInst() return false for these MachineInstr, allowing getInstSizeInBytes() to return a value and EstimateFunctionSizeInBytes() to work as intended.

Can you update the title to say make "Mark MachineInstr with FrameIndex as not compressible". The phrase "incomplete MachineInstr" is not very clear. I would interpret it as a MachineInstr with less operands than it's supposed to have. A MachineInstr with a FrameIndex is a completely valid MachineInstr so I wouldn't call it incomplete.

piggynl retitled this revision from [TableGen] Mark incomplete MachineInstr as not compressible to [TableGen] Mark MachineInstr with FrameIndex as not compressible.Aug 2 2022, 11:48 AM
piggynl edited the summary of this revision. (Show Details)

Can you update the title to say make "Mark MachineInstr with FrameIndex as not compressible". The phrase "incomplete MachineInstr" is not very clear. I would interpret it as a MachineInstr with less operands than it's supposed to have. A MachineInstr with a FrameIndex is a completely valid MachineInstr so I wouldn't call it incomplete.

Thanks for the correction! I updated the revision's summary as well.

Ping :) Do we have any updates on this?

piggynl retitled this revision from [TableGen] Mark MachineInstr with FrameIndex as not compressible to [RISCV][TableGen] Mark MachineInstr with FrameIndex as not compressible.Aug 17 2022, 6:48 AM

Ping for kindly review :)

I find this should be target dependent.

luismarques accepted this revision.Aug 23 2022, 4:24 PM
This revision is now accepted and ready to land.Aug 23 2022, 4:24 PM