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

[piggynl]

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.

[piggynl]

Update test.

[piggynl]

Ping for kindly review :)

[luismarques]

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]

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.

[craig.topper]

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]

In D129999#3694257, @craig.topper wrote:

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.

[piggynl]

Ping :) Do we have any updates on this?

[piggynl]

Ping for kindly review :)

I find this should be target dependent.

llvm/utils/TableGen/CompressInstEmitter.cpp

for (auto &CompressPat : CompressPatterns) {
     }
 
     // Start Source Inst operands validation.
     unsigned OpNo = 0;
     for (OpNo = 0; OpNo < Source.Operands.size(); ++OpNo) {
       if (SourceOperandMap[OpNo].TiedOpIdx != -1) {
         if (Source.Operands[OpNo].Rec->isSubClassOf("RegisterClass"))
           CondStream.indent(6)
-              << "(MI.getOperand(" << OpNo << ").getReg() ==  MI.getOperand("
+              << "(MI.getOperand(" << OpNo << ").isReg()) && (MI.getOperand("
+              << SourceOperandMap[OpNo].TiedOpIdx << ").isReg()) &&\n"
+              << "      (MI.getOperand(" << OpNo
+              << ").getReg() ==  MI.getOperand("
               << SourceOperandMap[OpNo].TiedOpIdx << ").getReg()) &&\n";
         else
           PrintFatalError("Unexpected tied operand types!\n");
       }
       // Check for fixed immediates\registers in the source instruction.
       switch (SourceOperandMap[OpNo].Kind) {
       case OpData::Operand:
         // We don't need to do anything for source instruction operand checks.
         break;
       case OpData::Imm:
         CondStream.indent(6)
             << "(MI.getOperand(" << OpNo << ").isImm()) &&\n"
             << "      (MI.getOperand(" << OpNo
             << ").getImm() == " << SourceOperandMap[OpNo].Data.Imm << ") &&\n";
         break;
       case OpData::Reg: {
         Record *Reg = SourceOperandMap[OpNo].Data.Reg;
         CondStream.indent(6)
-            << "(MI.getOperand(" << OpNo << ").getReg() == " << TargetName
+            << "(MI.getOperand(" << OpNo << ").isReg()) &&\n"
+            << "      (MI.getOperand(" << OpNo << ").getReg() == " << TargetName
             << "::" << Reg->getName() << ") &&\n";
         break;
       }
       }
     }
     CodeStream.indent(6) << "// " << Dest.AsmString << "\n";
     if (CompressOrUncompress)
       CodeStream.indent(6) << "OutInst.setOpcode(" << TargetName
                            << "::" << Dest.TheDef->getName() << ");\n";
     OpNo = 0;
     for (const auto &DestOperand : Dest.Operands) {
       CodeStream.indent(6) << "// Operand: " << DestOperand.Name << "\n";
       switch (DestOperandMap[OpNo].Kind) {
       case OpData::Operand: {
         unsigned OpIdx = DestOperandMap[OpNo].Data.Operand;
         // Check that the operand in the Source instruction fits
         // the type for the Dest instruction.
         if (DestOperand.Rec->isSubClassOf("RegisterClass")) {
           NeedMRI = true;
           // This is a register operand. Check the register class.
           // Don't check register class if this is a tied operand, it was done
           // for the operand its tied to.
           if (DestOperand.getTiedRegister() == -1)
-            CondStream.indent(6) << "(MRI.getRegClass(" << TargetName
-                                 << "::" << DestOperand.Rec->getName()
-                                 << "RegClassID).contains(MI.getOperand("
-                                 << OpIdx << ").getReg())) &&\n";
+            CondStream.indent(6)
+                << "(MI.getOperand(" << OpIdx << ").isReg()) &&\n"
+                << "      (MRI.getRegClass(" << TargetName
+                << "RegClassID).contains(MI.getOperand(" << OpIdx
+                << ").getReg())) &&\n";
 
           if (CompressOrUncompress)
             CodeStream.indent(6)
                 << "OutInst.addOperand(MI.getOperand(" << OpIdx << "));\n";
         } else {
           // Handling immediate operands.
           if (CompressOrUncompress) {
             unsigned Entry =