[X86][tablgen] Consider the mnemonic when auto-generating memory folding table

[skan]

Summary

Intuitively, the memory folding pair should have the same mnemonic.

This patch removes

{X86::SENDUIPI,X86::VMXON}

in the auto-generated table.
And NotMemoryFoldable for TPAUSE and CLWB can be saved.

{X86::MOVLHPSrr,X86::MOVHPSrm}
{X86::VMOVLHPSZrr,X86::VMOVHPSZ128rm}
{X86::VMOVLHPSrr,X86::VMOVHPSrm}

It seems the three pairs above are mistakenly killed.
But we can add them back manually later.

[skan]

As craig says, there is already a big test coverage hole on the hand-written memory folding tables. To make something easier, I have the following plan:

1. Gradually fix the gap between auto-generated folding table and the hand-written one
2. When the two are totally same, I will use the auto-generated table by default add a LIT test to check each entry of the table (sth like llvm/utils/update_llc_test_checks.py would be added)

Then we can prevent the possible regression in the future.

[Amir]

Should this diff be split into two: one with adding getMnemonic (NFC), and the other with changes to X86FoldTablesEmitter?

[skan]

Rebase

[skan]

In D122477#3407909, @Amir wrote:

Should this diff be split into two: one with adding getMnemonic (NFC), and the other with changes to X86FoldTablesEmitter?

Done

[craig.topper]

Do you intend to replace the other cases that use NotMemoryFoldable in the .td file which was how we used to fix this issue. See the multiple cases already in X86InstrVMX.td

llvm/utils/TableGen/X86FoldTablesEmitter.cpp

281

fileds -> fields

[skan]

In D122477#3409372, @craig.topper wrote:

Do you intend to replace the other cases that use NotMemoryFoldable in the .td file which was how we used to fix this issue. See the multiple cases already in X86InstrVMX.td

I can see you added them in https://reviews.llvm.org/rG66572df76eb9acf5c6b84c573541065cb29ce2ff
I think most of NotMemoryFoldable can be saved by checking the SchedRW is not WriteNop or WriteSystem. Do you mind I do it in another NFC patch?

[skan]

Rebase and fix typo

[craig.topper]

In D122477#3409403, @skan wrote:

In D122477#3409372, @craig.topper wrote:

Do you intend to replace the other cases that use NotMemoryFoldable in the .td file which was how we used to fix this issue. See the multiple cases already in X86InstrVMX.td

I can see you added them in https://reviews.llvm.org/rG66572df76eb9acf5c6b84c573541065cb29ce2ff
I think most of NotMemoryFoldable can be saved by checking the SchedRW is not WriteNop or WriteSystem. Do you mind I do it in another NFC patch?

As I said I was primarily asking about the ones in X86InstrVMX.td that are the exact same issue as VMXON and SENDUIPI. I just want us to solve the same problems in the same way.

[skan]

In D122477#3409403, @skan wrote:

In D122477#3409372, @craig.topper wrote:

Do you intend to replace the other cases that use NotMemoryFoldable in the .td file which was how we used to fix this issue. See the multiple cases already in X86InstrVMX.td

I can see you added them in https://reviews.llvm.org/rG66572df76eb9acf5c6b84c573541065cb29ce2ff
I think most of NotMemoryFoldable can be saved by checking the SchedRW is not WriteNop or WriteSystem. Do you mind I do it in another NFC patch?

Some of system instructions have the same mnemonic, e.g VMWRITE64rr, VMWRITE64rm while they shouldn't be added into the folding table. That's why I'd like to do it in another NFC patch.

[skan]

In D122477#3409416, @craig.topper wrote:

In D122477#3409403, @skan wrote:

In D122477#3409372, @craig.topper wrote:

Do you intend to replace the other cases that use NotMemoryFoldable in the .td file which was how we used to fix this issue. See the multiple cases already in X86InstrVMX.td

I can see you added them in https://reviews.llvm.org/rG66572df76eb9acf5c6b84c573541065cb29ce2ff
I think most of NotMemoryFoldable can be saved by checking the SchedRW is not WriteNop or WriteSystem. Do you mind I do it in another NFC patch?

As I said I was primarily asking about the ones in X86InstrVMX.td that are the exact same issue as VMXON and SENDUIPI. I just want us to solve the same problems in the same way.

Okay, I will update the patch soon.

[craig.topper]

In D122477#3409418, @skan wrote:

In D122477#3409416, @craig.topper wrote:

In D122477#3409403, @skan wrote:

In D122477#3409372, @craig.topper wrote:

Do you intend to replace the other cases that use NotMemoryFoldable in the .td file which was how we used to fix this issue. See the multiple cases already in X86InstrVMX.td

I can see you added them in https://reviews.llvm.org/rG66572df76eb9acf5c6b84c573541065cb29ce2ff
I think most of NotMemoryFoldable can be saved by checking the SchedRW is not WriteNop or WriteSystem. Do you mind I do it in another NFC patch?

As I said I was primarily asking about the ones in X86InstrVMX.td that are the exact same issue as VMXON and SENDUIPI. I just want us to solve the same problems in the same way.

Okay, I will update the patch soon.

Maybe it's not InstrVMX that has the overlap. But I remember something overlapped. It might be TPAUSE and CLWB

[skan]

Remove redundant NotMemoryFoldable

[skan]

In D122477#3409420, @craig.topper wrote:

In D122477#3409418, @skan wrote:

In D122477#3409416, @craig.topper wrote:

In D122477#3409403, @skan wrote:

In D122477#3409372, @craig.topper wrote:

Do you intend to replace the other cases that use NotMemoryFoldable in the .td file which was how we used to fix this issue. See the multiple cases already in X86InstrVMX.td

I can see you added them in https://reviews.llvm.org/rG66572df76eb9acf5c6b84c573541065cb29ce2ff
I think most of NotMemoryFoldable can be saved by checking the SchedRW is not WriteNop or WriteSystem. Do you mind I do it in another NFC patch?

As I said I was primarily asking about the ones in X86InstrVMX.td that are the exact same issue as VMXON and SENDUIPI. I just want us to solve the same problems in the same way.

Okay, I will update the patch soon.

Maybe it's not InstrVMX that has the overlap. But I remember something overlapped. It might be TPAUSE and CLWB

You're right! Done.

[skan]

Ping

[Amir]

Extra mnemonic check looks reasonable to me.

[skan]

Thanks @Amir !

Does this patch look good to you? @craig.topper

llvm/lib/Target/X86/CMakeLists.txt

 tablegen(LLVM X86GenGlobalISel.inc -gen-global-isel)
 tablegen(LLVM X86GenInstrInfo.inc -gen-instr-info)
 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)
+  tablegen(LLVM X86GenFoldTables.inc -gen-x86-fold-tables -asmwriternum=1)
 endif()
 
 add_public_tablegen_target(X86CommonTableGen)
 
 set(sources
   X86AsmPrinter.cpp
   X86AvoidTrailingCall.cpp
   X86CallFrameOptimization.cpp

llvm/lib/Target/X86/X86InstrInfo.td

let SchedRW = [WriteSystem] in {
   let Uses = [EAX, EDX], Defs = [EFLAGS] in {
     def UMWAIT : I<0xAE, MRM6r,
                      (outs), (ins GR32orGR64:$src), "umwait\t$src",
                      [(set EFLAGS, (X86umwait GR32orGR64:$src, EDX, EAX))]>,
                      XD, Requires<[HasWAITPKG]>;
     def TPAUSE : I<0xAE, MRM6r,
                      (outs), (ins GR32orGR64:$src), "tpause\t$src",
                      [(set EFLAGS, (X86tpause GR32orGR64:$src, EDX, EAX))]>,
-                     PD, Requires<[HasWAITPKG]>, NotMemoryFoldable;
+                     PD, Requires<[HasWAITPKG]>;
   }
 } // SchedRW
 
 //===----------------------------------------------------------------------===//
 // MOVDIRI - Move doubleword/quadword as direct store
 //
 let SchedRW = [WriteStore] in {
 def MOVDIRI32 : I<0xF9, MRMDestMem, (outs), (ins i32mem:$dst, GR32:$src),

 //
 
 let Predicates = [HasCLFLUSHOPT], SchedRW = [WriteLoad] in
 def CLFLUSHOPT : I<0xAE, MRM7m, (outs), (ins i8mem:$src),
                    "clflushopt\t$src", [(int_x86_clflushopt addr:$src)]>, PD;
 
 let Predicates = [HasCLWB], SchedRW = [WriteLoad] in
 def CLWB       : I<0xAE, MRM6m, (outs), (ins i8mem:$src), "clwb\t$src",
-                   [(int_x86_clwb addr:$src)]>, PD, NotMemoryFoldable;
+                   [(int_x86_clwb addr:$src)]>, PD;
 
 let Predicates = [HasCLDEMOTE], SchedRW = [WriteLoad] in
 def CLDEMOTE : I<0x1C, MRM0m, (outs), (ins i8mem:$src), "cldemote\t$src",
                    [(int_x86_cldemote addr:$src)]>, PS;
 
 //===----------------------------------------------------------------------===//
 // Subsystems.
 //===----------------------------------------------------------------------===//

llvm/utils/TableGen/X86FoldTablesEmitter.cpp

getAltRegInst(const CodeGenInstruction *I, const RecordKeeper &Records,
   CodeGenInstruction &AltRegInst = Target.getInstruction(AltRegInstRec);
   return &AltRegInst;
 }
 
 // Function object - Operator() returns true if the given VEX instruction
 // matches the EVEX instruction of this object.
 class IsMatch {
   const CodeGenInstruction *MemInst;
+  unsigned Variant;
 
 public:
-  IsMatch(const CodeGenInstruction *Inst, const RecordKeeper &Records)
-      : MemInst(Inst) {}
+  IsMatch(const CodeGenInstruction *Inst, unsigned V)
+      : MemInst(Inst), Variant(V) {}
 
   bool operator()(const CodeGenInstruction *RegInst) {
     X86Disassembler::RecognizableInstrBase RegRI(*RegInst);
     X86Disassembler::RecognizableInstrBase MemRI(*MemInst);
     const Record *RegRec = RegInst->TheDef;
     const Record *MemRec = MemInst->TheDef;
 
     // EVEX_B means different things for memory and register forms.
     if (RegRI.HasEVEX_B != 0 || MemRI.HasEVEX_B != 0)
       return false;
 
     // Instruction's format - The register form's "Form" field should be
     // the opposite of the memory form's "Form" field.
     if (!areOppositeForms(RegRI.Form, MemRI.Form))
       return false;
 
+    // X86 encoding is crazy, e.g
+    //
+    // f3 0f c7 30       vmxon   (%rax)
+    // f3 0f c7 f0       senduipi        %rax
+    //
+    // This two instruction have similiar encoding fields but are unrelated
+    if (X86Disassembler::getMnemonic(MemInst, Variant) !=
+        X86Disassembler::getMnemonic(RegInst, Variant))
+      return false;
+
     // Return false if one (at least) of the encoding fields of both
     // instructions do not match.
     if (RegRI.Encoding != MemRI.Encoding || RegRI.Opcode != MemRI.Opcode ||
         RegRI.OpPrefix != MemRI.OpPrefix || RegRI.OpMap != MemRI.OpMap ||
         RegRI.OpSize != MemRI.OpSize || RegRI.AdSize != MemRI.AdSize ||
         RegRI.HasREX_W != MemRI.HasREX_W ||
         RegRI.HasVEX_4V != MemRI.HasVEX_4V ||
         RegRI.HasVEX_L != MemRI.HasVEX_L ||
for (const CodeGenInstruction *Inst : NumberedInstructions) {
     if (hasMemoryFormat(Rec))
       MemInsts.push_back(Inst);
     else if (hasRegisterFormat(Rec)) {
       uint8_t Opc = getValueFromBitsInit(Rec->getValueAsBitsInit("Opcode"));
       RegInsts[Opc].push_back(Inst);
     }
   }
 
+  Record *AsmWriter = Target.getAsmWriter();
+  unsigned Variant = AsmWriter->getValueAsInt("Variant");
   // For each memory form instruction, try to find its register form
   // instruction.
   for (const CodeGenInstruction *MemInst : MemInsts) {
     uint8_t Opc =
         getValueFromBitsInit(MemInst->TheDef->getValueAsBitsInit("Opcode"));
 
     auto RegInstsIt = RegInsts.find(Opc);
     if (RegInstsIt == RegInsts.end())
       continue;
 
     // Two forms (memory & register) of the same instruction must have the same
     // opcode. try matching only with register form instructions with the same
     // opcode.
     std::vector<const CodeGenInstruction *> &OpcRegInsts = RegInstsIt->second;
 
-    auto Match = find_if(OpcRegInsts, IsMatch(MemInst, Records));
+    auto Match = find_if(OpcRegInsts, IsMatch(MemInst, Variant));
     if (Match != OpcRegInsts.end()) {
       const CodeGenInstruction *RegInst = *Match;
       // If the matched instruction has it's "FoldGenRegForm" set, map the
       // memory form instruction to the register form instruction pointed by
       // this field
       if (RegInst->TheDef->isValueUnset("FoldGenRegForm")) {
         updateTables(RegInst, MemInst);
       } else {