[X86] Add In64BitMode for MOVSX64/MOVZX64 instructions

[HaohaiWen]

Summary

REX_W prefix requires In64BitMode

[craig.topper]

I might be wrong but I think we are often missing In64BitMode on instructions that have a GR64 operand.

Adding them might increase the size of the table in X86GenDAGISel.inc

Are you seeing a functional issue that this fixes?

[HaohaiWen]

Are you seeing a functional issue that this fixes?

I'm fixing a AlderlakeP schedmodel issue: https://github.com/llvm/llvm-project/issues/58792.
To fix it, I need to use AsmMatcherEmitter to match CodeGenOnly but encodable instructions like CVTSD2SI64rm_Int.
This trigged assertion in X86AsmParser::validateInstruction:

if (UsesRex && HReg != X86::NoRegister) {
  StringRef RegName = X86IntelInstPrinter::getRegisterName(HReg);
  return Error(Ops[0]->getStartLoc(),
               "can't encode '" + RegName + "' in an instruction requiring "
               "REX prefix");
}

By validating hasREX_W and predicates in tblgen, I found there're 245 instructions has REX_W prefix but without In64BitMode predicate.

[skan]

Hi @craig.topper , why do you think this may increase the size of the table in X86GenDAGISel.inc?
@HaohaiWen Could you check how much the table grows with this patch?

[pengfei]

In D138639#3948608, @skan wrote:

Hi @craig.topper , why do you think this may increase the size of the table in X86GenDAGISel.inc?
@HaohaiWen Could you check how much the table grows with this patch?

I think it will increase 2 bytes for each pattern:

/*Scope*/ 13, /*->133335*/
 OPC_CheckChild0Type, MVT::i32,
 OPC_CheckType, MVT::i64,
 OPC_CheckPatternPredicate, 8, // (Subtarget->is64Bit())     <-- increase here.
 OPC_MorphNodeTo1, TARGET_VAL(X86::MOVSX64rr32), 0,
     MVT::i64, 1/*#Ops*/, 0,
 // Src: (sext:{ *:[i64] } GR32:{ *:[i32] }:$src) - Complexity = 3
 // Dst: (MOVSX64rr32:{ *:[i64] } GR32:{ *:[i32] }:$src)

[craig.topper]

In D138639#3948601, @HaohaiWen wrote:

Are you seeing a functional issue that this fixes?

I'm fixing a AlderlakeP schedmodel issue: https://github.com/llvm/llvm-project/issues/58792.
To fix it, I need to use AsmMatcherEmitter to match CodeGenOnly but encodable instructions like CVTSD2SI64rm_Int.

I'm not following. It's already not a CodeGenOnly instruction. I see this in the asm matcher table

{ 2010 /* cvtsd2si */, X86::CVTSD2SI64rm_Int, Convert__Reg1_1__Mem645_0, AMFBS_None, { MCK_Mem64, MCK_GR64 }, }

Isn't X86::CVTSD2SI64rm the CodeGenOnly instruction?

[HaohaiWen]

Isn't X86::CVTSD2SI64rm the CodeGenOnly instruction?

Yes, CVTSD2SI64rm isCodeGenOnly and CVTSD2SI64rm_Int is not.
I'm trying to auto gen asm enumeration for each encodable instructions. This relies on predicates to indicate the mode {16bit, 32bit, 64bit}.

[craig.topper]

In D138639#3950081, @HaohaiWen wrote:

Isn't X86::CVTSD2SI64rm the CodeGenOnly instruction?

Yes, CVTSD2SI64rm isCodeGenOnly and CVTSD2SI64rm_Int is not.
I'm trying to auto gen asm enumeration for each encodable instructions. This relies on predicates to indicate the mode {16bit, 32bit, 64bit}.

We’ve been assuming the assembler would never parse a GR64 register name outside of 64-bit mode which is why the predicates were omitted.

[craig.topper]

In D138639#3948601, @HaohaiWen wrote:

Are you seeing a functional issue that this fixes?

I'm fixing a AlderlakeP schedmodel issue: https://github.com/llvm/llvm-project/issues/58792.
To fix it, I need to use AsmMatcherEmitter to match CodeGenOnly but encodable instructions like CVTSD2SI64rm_Int.
This trigged assertion in X86AsmParser::validateInstruction:

if (UsesRex && HReg != X86::NoRegister) {
  StringRef RegName = X86IntelInstPrinter::getRegisterName(HReg);
  return Error(Ops[0]->getStartLoc(),
               "can't encode '" + RegName + "' in an instruction requiring "
               "REX prefix");
}

How does adding In64BitMode avoid this error? This error occurs when AH/BH/CH/DH are passed to an instruction that uses a REX prefix. Are you using In64BitMode to avoid passing AH/BH/CH/DH registers?

[HaohaiWen]

How does adding In64BitMode avoid this error? This error occurs when AH/BH/CH/DH are passed to an instruction that uses a REX prefix. Are you using In64BitMode to avoid passing AH/BH/CH/DH registers?

In fact, the issue is, I relied on predicates to auto gen asm. Let's take CVTSD2SI64rm as an example.
Since it have no predicates, I made assumption its encodable in all modes and then try to gen 16,32,64 bit asm string (by adding {.code16 .code32 . code64) enumeration and encode it with llvm-mc. Then fed the encoding into llvm-mc to decode in order to find all matchable llvm opcodes.
That's how I found this predicates error.

[pengfei]

In D138639#3950086, @HaohaiWen wrote:

How does adding In64BitMode avoid this error? This error occurs when AH/BH/CH/DH are passed to an instruction that uses a REX prefix. Are you using In64BitMode to avoid passing AH/BH/CH/DH registers?

In fact, the issue is, I relied on predicates to auto gen asm. Let's take CVTSD2SI64rm as an example.
Since it have no predicates, I made assumption its encodable in all modes and then try to gen 16,32,64 bit asm string (by adding {.code16 .code32 . code64) enumeration and encode it with llvm-mc. Then fed the encoding into llvm-mc to decode in order to find all matchable llvm opcodes.
That's how I found this predicates error.

Can we ignore those with isCodeGenOnly. I think they are just duplications of the non codegen only ones from the perspective of encoding.

[HaohaiWen]

Can we ignore those with isCodeGenOnly. I think they are just duplications of the non codegen only ones from the perspective of encoding.

Of course we can ignore it in almost all cases because they'll never be generated to asm printer.
However we should describe them correctly in schedule model. In fact, current schedtool D130897 only emit scheduling info for not CodeGenOnly instruction. That means scheduling info for CodeGenOnly instructions like CVTSD2SI64rm may be not correct, although it should be same with CVTSD2SI64rm_Int.
I'm working on fixing that, this requires correct mode predicates for CodeGenOnly and encodable instructions.

[craig.topper]

In D138639#3950123, @HaohaiWen wrote:

Can we ignore those with isCodeGenOnly. I think they are just duplications of the non codegen only ones from the perspective of encoding.

Of course we can ignore it in almost all cases because they'll never be generated to asm printer.
However we should describe them correctly in schedule model. In fact, current schedtool D130897 only emit scheduling info for not CodeGenOnly instruction. That means scheduling info for CodeGenOnly instructions like CVTSD2SI64rm may be not correct, although it should be same with CVTSD2SI64rm_Int.
I'm working on fixing that, this requires correct mode predicates for CodeGenOnly and encodable instructions.

Is it not possible to use the encoding information in TSFlags rather than going through the assembly parser? Your patches for schedtool seem very coupled to the names of operand classes and other things. It looks like it will require updates often.

[HaohaiWen]

Is it not possible to use the encoding information in TSFlags rather than going through the assembly parser? Your patches for schedtool seem very coupled to the names of operand classes and other things. It looks like it will require updates often.

The asm enumeration code and asm matcher patch as well as xed patch are used to build map between llvm opcode <-> Xed info <-> uops.info data / other scheduling info data source. Do you have any suggestion to build this map?
IsaSet in Xed info can also be used to identify whether a llvm opcode is supported by specific target. LLVM predicates can't determine that precisely.

Apart from that, I think we'd better fix wrong predicates.

[craig.topper]

In D138639#3950133, @HaohaiWen wrote:

Is it not possible to use the encoding information in TSFlags rather than going through the assembly parser? Your patches for schedtool seem very coupled to the names of operand classes and other things. It looks like it will require updates often.

The asm enumeration code and asm matcher patch as well as xed patch are used to build map between llvm opcode <-> Xed info <-> uops.info data / other scheduling info data source. Do you have any suggestion to build this map?
IsaSet in Xed info can also be used to identify whether a llvm opcode is supported by specific target. LLVM predicates can't determine that precisely.

I don’t know anything about xed. What does it require?

Apart from that, I think we'd better fix wrong predicates.

Be careful using the word “wrong”. The current implementation was intentional as it saves space in some generated tables.

[skan]

In D138639#3950133, @HaohaiWen wrote:

Is it not possible to use the encoding information in TSFlags rather than going through the assembly parser? Your patches for schedtool seem very coupled to the names of operand classes and other things. It looks like it will require updates often.

The asm enumeration code and asm matcher patch as well as xed patch are used to build map between llvm opcode <-> Xed info <-> uops.info data / other scheduling info data source. Do you have any suggestion to build this map?
IsaSet in Xed info can also be used to identify whether a llvm opcode is supported by specific target. LLVM predicates can't determine that precisely.

Apart from that, I think we'd better fix wrong predicates.

I think Craig means you can use the REX_W bit in TSFlags to assist you in Predicate check, e.g Condition = In64BitMode || HasREX_W.

[HaohaiWen]

I don’t know anything about xed. What does it require?

AFAIK, Input to it is normally encoding or asm string. That's why I need to enumerate asm string and encode it for each llvm opcodes.

[skan]

In D138639#3950140, @craig.topper wrote:

In D138639#3950133, @HaohaiWen wrote:

Is it not possible to use the encoding information in TSFlags rather than going through the assembly parser? Your patches for schedtool seem very coupled to the names of operand classes and other things. It looks like it will require updates often.

The asm enumeration code and asm matcher patch as well as xed patch are used to build map between llvm opcode <-> Xed info <-> uops.info data / other scheduling info data source. Do you have any suggestion to build this map?
IsaSet in Xed info can also be used to identify whether a llvm opcode is supported by specific target. LLVM predicates can't determine that precisely.

I don’t know anything about xed. What does it require?

Apart from that, I think we'd better fix wrong predicates.

Be careful using the word “wrong”. The current implementation was intentional as it saves space in some generated tables.

@craig.topper I'm not following. I also see that mayLoad, mayStore is omitted for x86 instructions with memory operands. And when the first operand is memory, the instructions is assumed to load/store, otherwise it's assumed to load only. Is it intentional or just for a slack off?

[craig.topper]

In D138639#3950146, @skan wrote:

In D138639#3950140, @craig.topper wrote:

In D138639#3950133, @HaohaiWen wrote:

Is it not possible to use the encoding information in TSFlags rather than going through the assembly parser? Your patches for schedtool seem very coupled to the names of operand classes and other things. It looks like it will require updates often.

The asm enumeration code and asm matcher patch as well as xed patch are used to build map between llvm opcode <-> Xed info <-> uops.info data / other scheduling info data source. Do you have any suggestion to build this map?
IsaSet in Xed info can also be used to identify whether a llvm opcode is supported by specific target. LLVM predicates can't determine that precisely.

I don’t know anything about xed. What does it require?

Apart from that, I think we'd better fix wrong predicates.

Be careful using the word “wrong”. The current implementation was intentional as it saves space in some generated tables.

@craig.topper I'm not following. I also see that mayLoad, mayStore is omitted for x86 instructions with memory operands. And when the first operand is memory, the instructions is assumed to load/store, otherwise it's assumed to load only. Is it intentional or just for a slack off?

Assumed where? In tablegen?

[skan]

In D138639#3950147, @craig.topper wrote:

In D138639#3950146, @skan wrote:

In D138639#3950140, @craig.topper wrote:

In D138639#3950133, @HaohaiWen wrote:

Is it not possible to use the encoding information in TSFlags rather than going through the assembly parser? Your patches for schedtool seem very coupled to the names of operand classes and other things. It looks like it will require updates often.

The asm enumeration code and asm matcher patch as well as xed patch are used to build map between llvm opcode <-> Xed info <-> uops.info data / other scheduling info data source. Do you have any suggestion to build this map?
IsaSet in Xed info can also be used to identify whether a llvm opcode is supported by specific target. LLVM predicates can't determine that precisely.

I don’t know anything about xed. What does it require?

Apart from that, I think we'd better fix wrong predicates.

Be careful using the word “wrong”. The current implementation was intentional as it saves space in some generated tables.

@craig.topper I'm not following. I also see that mayLoad, mayStore is omitted for x86 instructions with memory operands. And when the first operand is memory, the instructions is assumed to load/store, otherwise it's assumed to load only. Is it intentional or just for a slack off?

Assumed where? In tablegen?

llvm/lib/CodeGen/TargetInstrInfo.cpp

  auto Flags = MachineMemOperand::MONone;
  for (unsigned OpIdx : Ops)
    Flags |= MI.getOperand(OpIdx).isDef() ? MachineMemOperand::MOStore
                                          : MachineMemOperand::MOLoad;

...

    // Add a memory operand, foldMemoryOperandImpl doesn't do that.
    assert((!(Flags & MachineMemOperand::MOStore) ||
            NewMI->mayStore()) &&
           "Folded a def to a non-store!");
    assert((!(Flags & MachineMemOperand::MOLoad) ||
            NewMI->mayLoad()) &&
           "Folded a use to a non-load!");

llvm/lib/Target/X86/X86InstrInfo.cpp

if (I != nullptr) {
  unsigned Opcode = I->DstOp;
  bool FoldedLoad =
      isTwoAddrFold || (OpNum == 0 && I->Flags & TB_FOLDED_LOAD) || OpNum > 0;
  bool FoldedStore =
      isTwoAddrFold || (OpNum == 0 && I->Flags & TB_FOLDED_STORE);

[craig.topper]

In D138639#3950153, @skan wrote:

In D138639#3950147, @craig.topper wrote:

In D138639#3950146, @skan wrote:

In D138639#3950140, @craig.topper wrote:

In D138639#3950133, @HaohaiWen wrote:

Is it not possible to use the encoding information in TSFlags rather than going through the assembly parser? Your patches for schedtool seem very coupled to the names of operand classes and other things. It looks like it will require updates often.

The asm enumeration code and asm matcher patch as well as xed patch are used to build map between llvm opcode <-> Xed info <-> uops.info data / other scheduling info data source. Do you have any suggestion to build this map?
IsaSet in Xed info can also be used to identify whether a llvm opcode is supported by specific target. LLVM predicates can't determine that precisely.

I don’t know anything about xed. What does it require?

Apart from that, I think we'd better fix wrong predicates.

Be careful using the word “wrong”. The current implementation was intentional as it saves space in some generated tables.

@craig.topper I'm not following. I also see that mayLoad, mayStore is omitted for x86 instructions with memory operands. And when the first operand is memory, the instructions is assumed to load/store, otherwise it's assumed to load only. Is it intentional or just for a slack off?

Assumed where? In tablegen?

llvm/lib/CodeGen/TargetInstrInfo.cpp

  auto Flags = MachineMemOperand::MONone;
  for (unsigned OpIdx : Ops)
    Flags |= MI.getOperand(OpIdx).isDef() ? MachineMemOperand::MOStore
                                          : MachineMemOperand::MOLoad;

...

    // Add a memory operand, foldMemoryOperandImpl doesn't do that.
    assert((!(Flags & MachineMemOperand::MOStore) ||
            NewMI->mayStore()) &&
           "Folded a def to a non-store!");
    assert((!(Flags & MachineMemOperand::MOLoad) ||
            NewMI->mayLoad()) &&
           "Folded a use to a non-load!");

llvm/lib/Target/X86/X86InstrInfo.cpp

if (I != nullptr) {
  unsigned Opcode = I->DstOp;
  bool FoldedLoad =
      isTwoAddrFold || (OpNum == 0 && I->Flags & TB_FOLDED_LOAD) || OpNum > 0;
  bool FoldedStore =
      isTwoAddrFold || (OpNum == 0 && I->Flags & TB_FOLDED_STORE);

The first does that because we haven't folded the memory operand yet so the mayLoad/mayStore flag wouldn't be set. MI is a register only instruction at that point. I believe that function is used for folding reloads and spills for register allocation, so using isDef is accurate.

The second is taking a shortcut for the isTwoAddrFold case and the OpNum > 0 case. We could add the TB_FOLDED_LOAD and TB_FOLDED_STORE flags into the X86InstrFoldTables.cpp tables. I think I added that code to avoid updating all of the tables while fixing a bug in D89656. Thought similar assumptions exist here

X86MemUnfoldTable() {
  for (const X86MemoryFoldTableEntry &Entry : MemoryFoldTable2Addr)
    // Index 0, folded load and store, no alignment requirement.
    addTableEntry(Entry, TB_INDEX_0 | TB_FOLDED_LOAD | TB_FOLDED_STORE);

  for (const X86MemoryFoldTableEntry &Entry : MemoryFoldTable0)
    // Index 0, mix of loads and stores.
    addTableEntry(Entry, TB_INDEX_0);

  for (const X86MemoryFoldTableEntry &Entry : MemoryFoldTable1)
    // Index 1, folded load
    addTableEntry(Entry, TB_INDEX_1 | TB_FOLDED_LOAD);

  for (const X86MemoryFoldTableEntry &Entry : MemoryFoldTable2)
    // Index 2, folded load
    addTableEntry(Entry, TB_INDEX_2 | TB_FOLDED_LOAD);

  for (const X86MemoryFoldTableEntry &Entry : MemoryFoldTable3)
    // Index 3, folded load
    addTableEntry(Entry, TB_INDEX_3 | TB_FOLDED_LOAD);

  for (const X86MemoryFoldTableEntry &Entry : MemoryFoldTable4)
    // Index 4, folded load
    addTableEntry(Entry, TB_INDEX_4 | TB_FOLDED_LOAD);

  // Broadcast tables.
  for (const X86MemoryFoldTableEntry &Entry : BroadcastFoldTable2)
    // Index 2, folded broadcast
    addTableEntry(Entry, TB_INDEX_2 | TB_FOLDED_LOAD | TB_FOLDED_BCAST);

  for (const X86MemoryFoldTableEntry &Entry : BroadcastFoldTable3)
    // Index 3, folded broadcast
    addTableEntry(Entry, TB_INDEX_3 | TB_FOLDED_LOAD | TB_FOLDED_BCAST);

All of the flags could be moved into the tables and the second argument to addTableEntry could be removed.

My only concern is that it might increase the size and maybe compile time of the file.

[skan]

This comment was removed by skan.

[skan]

@craig.topper I see. Thanks!

llvm/lib/Target/X86/X86InstrExtension.td

def MOVSX32rm8_NOREX : I<0xBE, MRMSrcMem,
                          "movs{bl|x}\t{$src, $dst|$dst, $src}",
                          []>, TB, OpSize32, Sched<[WriteLoad]>;
 }
 
 // MOVSX64rr8 always has a REX prefix and it has an 8-bit register
 // operand, which makes it a rare instruction with an 8-bit register
 // operand that can never access an h register. If support for h registers
 // were generalized, this would require a special register class.
+let Predicates = [In64BitMode] in {
 def MOVSX64rr8 : RI<0xBE, MRMSrcReg, (outs GR64:$dst), (ins GR8 :$src),
                     "movs{bq|x}\t{$src, $dst|$dst, $src}",
                     [(set GR64:$dst, (sext GR8:$src))]>, TB,
                     Sched<[WriteALU]>;
 def MOVSX64rm8 : RI<0xBE, MRMSrcMem, (outs GR64:$dst), (ins i8mem :$src),
                     "movs{bq|x}\t{$src, $dst|$dst, $src}",
                     [(set GR64:$dst, (sextloadi64i8 addr:$src))]>,
                     TB, Sched<[WriteLoad]>;
 def MOVSX64rr16: RI<0xBF, MRMSrcReg, (outs GR64:$dst), (ins GR16:$src),
                     "movs{wq|x}\t{$src, $dst|$dst, $src}",
                     [(set GR64:$dst, (sext GR16:$src))]>, TB,
                     Sched<[WriteALU]>;
 def MOVSX64rm16: RI<0xBF, MRMSrcMem, (outs GR64:$dst), (ins i16mem:$src),
                     "movs{wq|x}\t{$src, $dst|$dst, $src}",
                     [(set GR64:$dst, (sextloadi64i16 addr:$src))]>,
                     TB, Sched<[WriteLoad]>;
 def MOVSX64rr32: RI<0x63, MRMSrcReg, (outs GR64:$dst), (ins GR32:$src),
                     "movs{lq|xd}\t{$src, $dst|$dst, $src}",
                     [(set GR64:$dst, (sext GR32:$src))]>,
-                    Sched<[WriteALU]>, Requires<[In64BitMode]>;
+                    Sched<[WriteALU]>;
 def MOVSX64rm32: RI<0x63, MRMSrcMem, (outs GR64:$dst), (ins i32mem:$src),
                     "movs{lq|xd}\t{$src, $dst|$dst, $src}",
                     [(set GR64:$dst, (sextloadi64i32 addr:$src))]>,
-                    Sched<[WriteLoad]>, Requires<[In64BitMode]>;
+                    Sched<[WriteLoad]>;
+}
 
 // These instructions exist as a consequence of operand size prefix having
 // control of the destination size, but not the input size. Only support them
 // for the disassembler.
 let isCodeGenOnly = 1, ForceDisassemble = 1, hasSideEffects = 0 in {
 def MOVSX16rr32: I<0x63, MRMSrcReg, (outs GR16:$dst), (ins GR32:$src),
                    "movs{lq|xd}\t{$src, $dst|$dst, $src}", []>,
                    Sched<[WriteALU]>, OpSize16, Requires<[In64BitMode]>;
 def MOVSX32rr32: I<0x63, MRMSrcReg, (outs GR32:$dst), (ins GR32:$src),
                    "movs{lq|xd}\t{$src, $dst|$dst, $src}", []>,
                    Sched<[WriteALU]>, OpSize32, Requires<[In64BitMode]>;
 let mayLoad = 1 in {
 def MOVSX16rm32: I<0x63, MRMSrcMem, (outs GR16:$dst), (ins i32mem:$src),
                    "movs{lq|xd}\t{$src, $dst|$dst, $src}", []>,
                    Sched<[WriteLoad]>, OpSize16, Requires<[In64BitMode]>;
 def MOVSX32rm32: I<0x63, MRMSrcMem, (outs GR32:$dst), (ins i32mem:$src),
                    "movs{lq|xd}\t{$src, $dst|$dst, $src}", []>,
                    Sched<[WriteLoad]>, OpSize32, Requires<[In64BitMode]>;
 } // mayLoad = 1
 } // isCodeGenOnly = 1, ForceDisassemble = 1, hasSideEffects = 0
 
 // movzbq and movzwq encodings for the disassembler
-let hasSideEffects = 0 in {
+let hasSideEffects = 0, Predicates = [In64BitMode] in {
 def MOVZX64rr8 : RI<0xB6, MRMSrcReg, (outs GR64:$dst), (ins GR8:$src),
                      "movz{bq|x}\t{$src, $dst|$dst, $src}", []>,
                      TB, Sched<[WriteALU]>;
 let mayLoad = 1 in
 def MOVZX64rm8 : RI<0xB6, MRMSrcMem, (outs GR64:$dst), (ins i8mem:$src),
                      "movz{bq|x}\t{$src, $dst|$dst, $src}", []>,
                      TB, Sched<[WriteLoad]>;
 def MOVZX64rr16 : RI<0xB7, MRMSrcReg, (outs GR64:$dst), (ins GR16:$src),