[X86][BOLT] Use getOperandType to determine memory access size

[Amir]

Summary

Generate INSTRINFO_OPERAND_TYPE table in X86GenInstrInfo.inc.

This diff adds support for instructions that were previously reported as having
memory access size 0. It replaces the heuristic of looking at instruction
register width to determine memory access width by instead checking the memory
operand type using tablegen-provided tables.

[Amir]

clang-format

[Amir]

bolt/lib/Target/X86/X86MCPlusBuilder.cpp

1011

getMemDataSize may be moved to Target/X86 files, but I'm not sure of any uses for it there.

[maksfb]

shrinkwrapping-lock.s is failing. Is this a true NFC?

[Amir]

In D126116#3554964, @maksfb wrote:

shrinkwrapping-lock.s is failing. Is this a true NFC?

It should be, but apparently it's not. I'll investigate.

[rafauler]

No, this diff is not NFC. It's actually pretty impactful (and thanks for working on this Amir).

If you change that, shrink wrapping will recognize way more instructions and will analyze more functions, causing the optimization to activate more often.

Because of that, we need to update tests. We may want to coordinate landing this together with my patchset of 5 diffs, so I can update all tests only once, instead of multiple times, one per diff.

[rafauler]

Let me try to elaborate on why this diff is important and has functional changes.

Previously, we were looking at register size to try to gauge how wide is the memory access: either source or destination register for a memory access. If we couldn't find one, we would say it is a "size zero" access. Returning "size 0" essentially disables all frame analyses because you have an unrecognized access to the stack. This diff looks at memory operands, which makes it cover a lot more cases.

We should probably add to this to the comments of this function so it becomes clear:

Classifying a stack access as *not* "SIMPLE" here means we don't know how to change this instruction memory access. It will disable any changes to the stack layout, so we can't do the most aggressive form of shrink wrapping. We must do so in a way that keeps the original stack layout. Otherwise you need to adjust the offset of all instructions accessing the stack: we can't do that anymore because there is one instruction that is not simple. There are other implications as well. We have heuristics to detect when a register is callee-saved and thus eligible for shrink wrapping. If you are restoring a register using a non-simple stack access, then it is classified as NOT callee-saved, and it disables shrink wrapping for *that* register (but not for others).

Classifying a stack access as "size 0" or detecting an indexed memory access (to address a vector, for example) here means we know there is a stack access, but we can't quite understand how wide is the access in bytes. This is very serious because we can't understand how memory accesses alias with each other for this function. This will essentially disable not only shrink wrapping but all frame analysis, it will fail it as "we don't understand this function and we give up on it". Because this diff changes the instructions classified as "size 0", it pretty much boosts how general are all frame optimizations.

Turns out that we were failing to classify the size of quite a few instructions, that's why I have the diff on https://reviews.llvm.org/D126112 to try to cover a few of the cases of non-simple stack accesses, but whose size should be different than zero. As you can see, those are instructions that lack a register operand. I also have one extra record for an instruction that does have a register, but it was wrongly classified as non-simple.

[rafauler]

Btw from BOLT perspective this diff LGTM. Regarding the changes to X86 target, those look minimal and rather benign to me. It is apparently using tablegen info (GET_INSTRINFO_OPERAND_TYPE) that is not used by any other backend that is in-tree, but TableGen supports generating this info and it is even in their testsuite. Even if it has a bug because it hasn't been used, I think it makes sense to try for us as it obviously won't break any existing code as we are the only users of this information. We should wait for someone responsible for the X86 target to approve this change, though.

[Amir]

This comment was removed by Amir.

[Amir]

Added Rafael's comment for isStackAccess

[Amir]

@rafaelauler:
Thank you for clarifications. I've added your comment verbatim to isStackAccess.
How do you want to proceed with this diff - should we let D126112 in first, or this one first?

[rafauler]

We should time it to land in the same day and then land changes to tests that compensates for the aggregate of all changes. Regarding the order, I don't have any preference, as long as both patchsets hit the repo in the same day.

[rafauler]

(I'm talking about a separate test repo, btw. The in-tree test should still be fixed individually for each diff to avoid breaking bisecting tools)

[skan]

bolt/lib/Target/X86/X86MCPlusBuilder.cpp

1013–1058

Could we avoid writing this table manually here? We already have similar things e.g X86Disassembler::getMemOperandSize? Maybe we can add one bit for class X86MemAsmOperand, and auto-generate this table.

llvm/lib/Target/X86/X86InstrInfo.cpp

9651 ↗

(On Diff #434131)

I am worried about this will increase the build time and memory usage a lot..
Should we put this into a bolt-only file?

[Amir]

bolt/lib/Target/X86/X86MCPlusBuilder.cpp

1013–1058

Will do

[Amir]

Address feedback

[skan]

LGTM

[Amir]

Use non-expanded memory operands

bolt/lib/Target/X86/X86MCPlusBuilder.cpp

 
 bool isADD64rr(const MCInst &Inst) { return Inst.getOpcode() == X86::ADD64rr; }
 
 bool isADDri(const MCInst &Inst) {
   return Inst.getOpcode() == X86::ADD64ri32 ||
          Inst.getOpcode() == X86::ADD64ri8;
 }
 
+#define GET_INSTRINFO_OPERAND_TYPES_ENUM
+#define GET_INSTRINFO_OPERAND_TYPE
+#define GET_INSTRINFO_MEM_OPERAND_SIZE
+#include "X86GenInstrInfo.inc"
+
 class X86MCPlusBuilder : public MCPlusBuilder {
 public:
   X86MCPlusBuilder(const MCInstrAnalysis *Analysis, const MCInstrInfo *Info,
                    const MCRegisterInfo *RegInfo)
       : MCPlusBuilder(Analysis, Info, RegInfo) {}
 
   std::unique_ptr<MCSymbolizer>
   createTargetSymbolizer(BinaryFunction &Function) const override {
case X86::TEST8ri:
           return true;
       }
       LLVM_FALLTHROUGH;
     default:
       return false;
     }
   }
 
+  static uint8_t getMemDataSize(const MCInst &Inst, int MemOpNo) {
+    using namespace llvm::X86;
+    int OpType = getOperandType(Inst.getOpcode(), MemOpNo);
+    return getMemOperandSize(OpType) / 8;
+  }
+
+  /// Classifying a stack access as *not* "SIMPLE" here means we don't know how
+  /// to change this instruction memory access. It will disable any changes to
+  /// the stack layout, so we can't do the most aggressive form of shrink
+  /// wrapping. We must do so in a way that keeps the original stack layout.
+  /// Otherwise you need to adjust the offset of all instructions accessing the
+  /// stack: we can't do that anymore because there is one instruction that is
+  /// not simple. There are other implications as well. We have heuristics to
+  /// detect when a register is callee-saved and thus eligible for shrink
+  /// wrapping. If you are restoring a register using a non-simple stack access,
+  /// then it is classified as NOT callee-saved, and it disables shrink wrapping
+  /// for *that* register (but not for others).
+  ///
+  /// Classifying a stack access as "size 0" or detecting an indexed memory
+  /// access (to address a vector, for example) here means we know there is a
+  /// stack access, but we can't quite understand how wide is the access in
+  /// bytes. This is very serious because we can't understand how memory
+  /// accesses alias with each other for this function. This will essentially
+  /// disable not only shrink wrapping but all frame analysis, it will fail it
+  /// as "we don't understand this function and we give up on it".
   bool isStackAccess(const MCInst &Inst, bool &IsLoad, bool &IsStore,
                      bool &IsStoreFromReg, MCPhysReg &Reg, int32_t &SrcImm,
                      uint16_t &StackPtrReg, int64_t &StackOffset, uint8_t &Size,
                      bool &IsSimple, bool &IsIndexed) const override {
     // Detect simple push/pop cases first
     if (int Sz = getPushSize(Inst)) {
       IsLoad = false;
       IsStore = true;
       IsStoreFromReg = true;
       StackPtrReg = X86::RSP;
       StackOffset = -Sz;
       Size = Sz;
       IsSimple = true;
       if (Inst.getOperand(0).isImm())
         SrcImm = Inst.getOperand(0).getImm();
       else if (Inst.getOperand(0).isReg())
         Reg = Inst.getOperand(0).getReg();
       else
         IsSimple = false;
 
       return true;
     }
     if (int Sz = getPopSize(Inst)) {
       IsLoad = true;
       IsStore = false;
       if (Inst.getNumOperands() == 0 || !Inst.getOperand(0).isReg()) {
         IsSimple = false;
       } else {
         Reg = Inst.getOperand(0).getReg();
         IsSimple = true;
       }
bool isStackAccess(const MCInst &Inst, bool &IsLoad, bool &IsStore,
     int MemOpNo = getMemoryOperandNo(Inst);
     const MCInstrDesc &MCII = Info->get(Inst.getOpcode());
     // If it is not dealing with a memory operand, we discard it
     if (MemOpNo == -1 || MCII.isCall())
       return false;
 
     switch (Inst.getOpcode()) {
     default: {
-      uint8_t Sz = 0;
       bool IsLoad = MCII.mayLoad();
       bool IsStore = MCII.mayStore();
       // Is it LEA? (deals with memory but is not loading nor storing)
       if (!IsLoad && !IsStore)
         return false;
-
-      // Try to guess data size involved in the load/store by looking at the
-      // register size. If there's no reg involved, return 0 as size, meaning
-      // we don't know.
-      for (unsigned I = 0, E = MCII.getNumOperands(); I != E; ++I) {
-        if (MCII.OpInfo[I].OperandType != MCOI::OPERAND_REGISTER)
-          continue;
-        if (static_cast<int>(I) >= MemOpNo && I < X86::AddrNumOperands)
-          continue;
-        Sz = RegInfo->getRegClass(MCII.OpInfo[I].RegClass).getSizeInBits() / 8;
-        break;
-      }
+      uint8_t Sz = getMemDataSize(Inst, MemOpNo);
       I = {Sz, IsLoad, IsStore, false, false};
       break;
     }
     case X86::MOV16rm: I = {2, true, false, false, true}; break;
     case X86::MOV32rm: I = {4, true, false, false, true}; break;
     case X86::MOV64rm: I = {8, true, false, false, true}; break;
     case X86::MOV16mr: I = {2, false, true, true, true};  break;
     case X86::MOV32mr: I = {4, false, true, true, true};  break;

llvm/lib/Target/X86/CMakeLists.txt

 add_llvm_component_group(X86 HAS_JIT)
 
 set(LLVM_TARGET_DEFINITIONS X86.td)
 
 tablegen(LLVM X86GenAsmMatcher.inc -gen-asm-matcher)
 tablegen(LLVM X86GenAsmWriter.inc -gen-asm-writer)
 tablegen(LLVM X86GenAsmWriter1.inc -gen-asm-writer -asmwriternum=1)
 tablegen(LLVM X86GenCallingConv.inc -gen-callingconv)
 tablegen(LLVM X86GenDAGISel.inc -gen-dag-isel)
 tablegen(LLVM X86GenDisassemblerTables.inc -gen-disassembler)
 tablegen(LLVM X86GenEVEX2VEXTables.inc -gen-x86-EVEX2VEX-tables)
 tablegen(LLVM X86GenExegesis.inc -gen-exegesis)
 tablegen(LLVM X86GenFastISel.inc -gen-fast-isel)
 tablegen(LLVM X86GenGlobalISel.inc -gen-global-isel)
-tablegen(LLVM X86GenInstrInfo.inc -gen-instr-info)
+tablegen(LLVM X86GenInstrInfo.inc -gen-instr-info
+                                  -instr-info-expand-mi-operand-info=0)
 tablegen(LLVM X86GenMnemonicTables.inc -gen-x86-mnemonic-tables -asmwriternum=1)
 tablegen(LLVM X86GenRegisterBank.inc -gen-register-bank)
 tablegen(LLVM X86GenRegisterInfo.inc -gen-register-info)
 tablegen(LLVM X86GenSubtargetInfo.inc -gen-subtarget)
 
 if (X86_GEN_FOLD_TABLES)
   tablegen(LLVM X86GenFoldTables.inc -gen-x86-fold-tables -asmwriternum=1)
 endif()