[TableGen] Use bitwise operations to access HwMode ID. #88377
Conversation
|
@llvm/pr-subscribers-mc @llvm/pr-subscribers-backend-risc-v Author: None (superZWT123) ChangesPatch is 32.26 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/88377.diff 14 Files Affected:
diff --git a/llvm/include/llvm/MC/MCSubtargetInfo.h b/llvm/include/llvm/MC/MCSubtargetInfo.h
index f172a799aa3331..70b14665ea9ebd 100644
--- a/llvm/include/llvm/MC/MCSubtargetInfo.h
+++ b/llvm/include/llvm/MC/MCSubtargetInfo.h
@@ -240,7 +240,23 @@ class MCSubtargetInfo {
return ProcFeatures;
}
- virtual unsigned getHwMode() const { return 0; }
+ /// HwMode ID will be stored as bits, allowing users to pull the specific
+ /// HwMode ID (like RegInfo HwMode ID) from the bits as needed. This enables
+ /// users to control multiple features with one hwmode (as previously) or use
+ /// different hwmodes to control different features.
+ enum HwModeType {
+ HwMode_Default, // Return the smallest HwMode ID of current subtarget.
+ HwMode_ValueType, // Return the HwMode ID that controls the ValueType.
+ HwMode_RegInfo, // Return the HwMode ID that controls the RegSizeInfo and
+ // SubRegRange.
+ HwMode_EncodingInfo // Return the HwMode ID that controls the EncodingInfo.
+ };
+
+ virtual unsigned getHwModeSet() const { return 0; }
+
+ virtual unsigned getHwMode(enum HwModeType type = HwMode_Default) const {
+ return 0;
+ }
/// Return the cache size in bytes for the given level of cache.
/// Level is zero-based, so a value of zero means the first level of
diff --git a/llvm/lib/Target/RISCV/CMakeLists.txt b/llvm/lib/Target/RISCV/CMakeLists.txt
index 8715403f3839a6..0d5205f5351ecd 100644
--- a/llvm/lib/Target/RISCV/CMakeLists.txt
+++ b/llvm/lib/Target/RISCV/CMakeLists.txt
@@ -7,7 +7,7 @@ tablegen(LLVM RISCVGenAsmWriter.inc -gen-asm-writer)
tablegen(LLVM RISCVGenCompressInstEmitter.inc -gen-compress-inst-emitter)
tablegen(LLVM RISCVGenMacroFusion.inc -gen-macro-fusion-pred)
tablegen(LLVM RISCVGenDAGISel.inc -gen-dag-isel)
-tablegen(LLVM RISCVGenDisassemblerTables.inc -gen-disassembler)
+tablegen(LLVM RISCVGenDisassemblerTables.inc -gen-disassembler --suppress-per-hwmode-duplicates=O1)
tablegen(LLVM RISCVGenInstrInfo.inc -gen-instr-info)
tablegen(LLVM RISCVGenMCCodeEmitter.inc -gen-emitter)
tablegen(LLVM RISCVGenMCPseudoLowering.inc -gen-pseudo-lowering)
diff --git a/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp b/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
index 6aadabdf1bc61a..7298526bb36f93 100644
--- a/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
+++ b/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
@@ -640,6 +640,8 @@ DecodeStatus RISCVDisassembler::getInstruction(MCInst &MI, uint64_t &Size,
TRY_TO_DECODE_FEATURE(
RISCV::FeatureStdExtZcmp, DecoderTableRVZcmp16,
"Zcmp table (16-bit Push/Pop & Double Move Instructions)");
+ TRY_TO_DECODE_AND_ADD_SP(STI.hasFeature(RISCV::FeatureEncodingTmp), DecoderTable_EncodingTmp16,
+ "For HwMode check");
TRY_TO_DECODE_AND_ADD_SP(true, DecoderTable16,
"RISCV_C table (16-bit Instruction)");
diff --git a/llvm/lib/Target/RISCV/RISCVFeatures.td b/llvm/lib/Target/RISCV/RISCVFeatures.td
index 794455aa730400..072649f4610f8d 100644
--- a/llvm/lib/Target/RISCV/RISCVFeatures.td
+++ b/llvm/lib/Target/RISCV/RISCVFeatures.td
@@ -1150,15 +1150,21 @@ def Feature32Bit
: SubtargetFeature<"32bit", "IsRV32", "true", "Implements RV32">;
def Feature64Bit
: SubtargetFeature<"64bit", "IsRV64", "true", "Implements RV64">;
+def FeatureEncodingTmp
+ : SubtargetFeature<"TmpE", "IsTmpE", "true", "Implements encoding tmp">;
def IsRV64 : Predicate<"Subtarget->is64Bit()">,
AssemblerPredicate<(all_of Feature64Bit),
"RV64I Base Instruction Set">;
def IsRV32 : Predicate<"!Subtarget->is64Bit()">,
AssemblerPredicate<(all_of (not Feature64Bit)),
"RV32I Base Instruction Set">;
+def IsTmpE : Predicate<"Subtarget->isETmp()">,
+ AssemblerPredicate<(all_of FeatureEncodingTmp),
+ "RV32I Encoding tmp">;
defvar RV32 = DefaultMode;
def RV64 : HwMode<"+64bit", [IsRV64]>;
+def EncodingTmp : HwMode<"+TmpE", [IsRV64]>;
def FeatureRVE
: SubtargetFeature<"e", "IsRVE", "true",
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoZcmop.td b/llvm/lib/Target/RISCV/RISCVInstrInfoZcmop.td
index dd13a07d606d04..23a0a6fb31f00e 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoZcmop.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoZcmop.td
@@ -11,6 +11,38 @@
//
//===----------------------------------------------------------------------===//
+multiclass tmpEclass<bits<3> imm3> {
+def NAME#commonE : InstructionEncoding {
+ field bits<16> Inst;
+ field bits<16> SoftFail = 0;
+ let Size = 2;
+ bit mayLoad = 0;
+ bit mayStore = 0;
+ bit hasSideEffects = 0;
+ let Inst{1-0} = 0b01;
+ let Inst{6-2} = 0;
+ let Inst{7} = 0b1; //differ
+ let Inst{10-8} = imm3;
+ let Inst{12-11} = 0;
+ let Inst{15-13} = 0b011;
+}
+
+def NAME#specialE : InstructionEncoding {
+ field bits<16> Inst;
+ field bits<16> SoftFail = 0;
+ let Size = 2;
+ bit mayLoad = 0;
+ bit mayStore = 0;
+ bit hasSideEffects = 0;
+ let Inst{1-0} = 0b01;
+ let Inst{6-2} = 0;
+ let Inst{7} = 0b0; //differ
+ let Inst{10-8} = imm3;
+ let Inst{12-11} = 0;
+ let Inst{15-13} = 0b011;
+}
+}
+
let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
class CMOPInst<bits<3> imm3, string opcodestr>
: RVInst16CI<0b011, 0b01, (outs), (ins), opcodestr, ""> {
@@ -18,15 +50,21 @@ class CMOPInst<bits<3> imm3, string opcodestr>
let Inst{7} = 1;
let Inst{10-8} = imm3;
let Inst{12-11} = 0;
+ let EncodingInfos = EncodingByHwMode<[DefaultMode, EncodingTmp],
+ [!cast<InstructionEncoding>(NAME#commonE),
+ !cast<InstructionEncoding>(NAME#specialE)]>;
}
// CMOP1, CMOP5 is used by Zicfiss.
let Predicates = [HasStdExtZcmop, NoHasStdExtZicfiss] in {
+ defm CMOP1 : tmpEclass<0>;
+ defm CMOP5 : tmpEclass<2>;
def CMOP1 : CMOPInst<0, "cmop.1">, Sched<[]>;
def CMOP5 : CMOPInst<2, "cmop.5">, Sched<[]>;
}
foreach n = [3, 7, 9, 11, 13, 15] in {
let Predicates = [HasStdExtZcmop] in
+ defm CMOP # n : tmpEclass<!srl(n, 1)>;
def CMOP # n : CMOPInst<!srl(n, 1), "cmop." # n>, Sched<[]>;
}
diff --git a/llvm/lib/Target/RISCV/RISCVProcessors.td b/llvm/lib/Target/RISCV/RISCVProcessors.td
index fd6d6078ec238b..cbb199ffb76585 100644
--- a/llvm/lib/Target/RISCV/RISCVProcessors.td
+++ b/llvm/lib/Target/RISCV/RISCVProcessors.td
@@ -60,7 +60,7 @@ def GENERIC_RV32 : RISCVProcessorModel<"generic-rv32",
GenericTuneInfo;
def GENERIC_RV64 : RISCVProcessorModel<"generic-rv64",
NoSchedModel,
- [Feature64Bit]>,
+ [Feature64Bit, FeatureEncodingTmp]>,
GenericTuneInfo;
// Support generic for compatibility with other targets. The triple will be used
// to change to the appropriate rv32/rv64 version.
diff --git a/llvm/lib/Target/RISCV/RISCVSubtarget.cpp b/llvm/lib/Target/RISCV/RISCVSubtarget.cpp
index d3236bb07d56d5..b4156df907ee97 100644
--- a/llvm/lib/Target/RISCV/RISCVSubtarget.cpp
+++ b/llvm/lib/Target/RISCV/RISCVSubtarget.cpp
@@ -100,11 +100,11 @@ RISCVSubtarget::RISCVSubtarget(const Triple &TT, StringRef CPU,
RVVVectorBitsMin(RVVVectorBitsMin), RVVVectorBitsMax(RVVVectorBitsMax),
FrameLowering(
initializeSubtargetDependencies(TT, CPU, TuneCPU, FS, ABIName)),
- InstrInfo(*this), RegInfo(getHwMode()), TLInfo(TM, *this) {
+ InstrInfo(*this), RegInfo(getHwMode(HwMode_RegInfo)), TLInfo(TM, *this) {
CallLoweringInfo.reset(new RISCVCallLowering(*getTargetLowering()));
Legalizer.reset(new RISCVLegalizerInfo(*this));
- auto *RBI = new RISCVRegisterBankInfo(getHwMode());
+ auto *RBI = new RISCVRegisterBankInfo(getHwMode(HwMode_RegInfo));
RegBankInfo.reset(RBI);
InstSelector.reset(createRISCVInstructionSelector(
*static_cast<const RISCVTargetMachine *>(&TM), *this, *RBI));
diff --git a/llvm/lib/Target/RISCV/RISCVSubtarget.h b/llvm/lib/Target/RISCV/RISCVSubtarget.h
index 85f8f5f654fe7c..28e9ff4c07f436 100644
--- a/llvm/lib/Target/RISCV/RISCVSubtarget.h
+++ b/llvm/lib/Target/RISCV/RISCVSubtarget.h
@@ -165,6 +165,7 @@ class RISCVSubtarget : public RISCVGenSubtargetInfo {
}
bool is64Bit() const { return IsRV64; }
+ bool isETmp() const { return IsTmpE; }
MVT getXLenVT() const {
return is64Bit() ? MVT::i64 : MVT::i32;
}
diff --git a/llvm/test/MC/RISCV/rvzcmop-valid.s b/llvm/test/MC/RISCV/rvzcmop-valid.s
index c6bb4a15808258..4e0ce25f73efa3 100644
--- a/llvm/test/MC/RISCV/rvzcmop-valid.s
+++ b/llvm/test/MC/RISCV/rvzcmop-valid.s
@@ -1,42 +1,50 @@
# RUN: llvm-mc %s -triple=riscv32 -mattr=+zcmop -show-encoding \
# RUN: | FileCheck -check-prefixes=CHECK-ASM,CHECK-ASM-AND-OBJ %s
# RUN: llvm-mc %s -triple=riscv64 -mattr=+zcmop -show-encoding \
-# RUN: | FileCheck -check-prefixes=CHECK-ASM,CHECK-ASM-AND-OBJ %s
+# RUN: | FileCheck -check-prefixes=CHECK-ASM-64,CHECK-ASM-AND-OBJ %s
# RUN: llvm-mc -filetype=obj -triple=riscv32 -mattr=+zcmop < %s \
-# RUN: | llvm-objdump --mattr=+zcmop -d -r - \
+# RUN: | llvm-objdump --triple=riscv32 --mattr=+zcmop -d -r - \
# RUN: | FileCheck --check-prefix=CHECK-ASM-AND-OBJ %s
# RUN: llvm-mc -filetype=obj -triple=riscv64 -mattr=+zcmop < %s \
-# RUN: | llvm-objdump --mattr=+zcmop -d -r - \
+# RUN: | llvm-objdump --triple=riscv64 --mattr=+zcmop -d -r - \
# RUN: | FileCheck --check-prefix=CHECK-ASM-AND-OBJ %s
# CHECK-ASM-AND-OBJ: cmop.1
# CHECK-ASM: encoding: [0x81,0x60]
+# CHECK-ASM-64: encoding: [0x01,0x60]
cmop.1
# CHECK-ASM-AND-OBJ: cmop.3
# CHECK-ASM: encoding: [0x81,0x61]
+# CHECK-ASM-64: encoding: [0x01,0x61]
cmop.3
# CHECK-ASM-AND-OBJ: cmop.5
# CHECK-ASM: encoding: [0x81,0x62]
+# CHECK-ASM-64: encoding: [0x01,0x62]
cmop.5
# CHECK-ASM-AND-OBJ: cmop.7
# CHECK-ASM: encoding: [0x81,0x63]
+# CHECK-ASM-64: encoding: [0x01,0x63]
cmop.7
# CHECK-ASM-AND-OBJ: cmop.9
# CHECK-ASM: encoding: [0x81,0x64]
+# CHECK-ASM-64: encoding: [0x01,0x64]
cmop.9
# CHECK-ASM-AND-OBJ: cmop.11
# CHECK-ASM: encoding: [0x81,0x65]
+# CHECK-ASM-64: encoding: [0x01,0x65]
cmop.11
# CHECK-ASM-AND-OBJ: cmop.13
# CHECK-ASM: encoding: [0x81,0x66]
+# CHECK-ASM-64: encoding: [0x01,0x66]
cmop.13
# CHECK-ASM-AND-OBJ: cmop.15
# CHECK-ASM: encoding: [0x81,0x67]
+# CHECK-ASM-64: encoding: [0x01,0x67]
cmop.15
diff --git a/llvm/test/TableGen/HwModeBitSet.td b/llvm/test/TableGen/HwModeBitSet.td
new file mode 100644
index 00000000000000..c642906f6f4a88
--- /dev/null
+++ b/llvm/test/TableGen/HwModeBitSet.td
@@ -0,0 +1,162 @@
+// RUN: llvm-tblgen -gen-register-info -register-info-debug -I %p/../../include %s -o /dev/null 2>&1 | FileCheck %s --check-prefix=CHECK-REG
+// RUN: llvm-tblgen -gen-subtarget -I %p/../../include %s -o - 2>&1 | FileCheck %s --check-prefix=CHECK-SUBTARGET
+
+include "llvm/Target/Target.td"
+
+def TestTargetInstrInfo : InstrInfo;
+
+def TestTarget : Target {
+ let InstructionSet = TestTargetInstrInfo;
+}
+
+def TestMode : HwMode<"+feat", []>;
+def TestMode1 : HwMode<"+feat1", []>;
+def TestMode2 : HwMode<"+feat2", []>;
+
+class MyReg<string n>
+ : Register<n> {
+ let Namespace = "Test";
+}
+
+class MyClass<int size, list<ValueType> types, dag registers>
+ : RegisterClass<"Test", types, size, registers> {
+ let Size = size;
+}
+
+def X0 : MyReg<"x0">;
+def X1 : MyReg<"x1">;
+def X2 : MyReg<"x2">;
+def X3 : MyReg<"x3">;
+def X4 : MyReg<"x4">;
+def X5 : MyReg<"x5">;
+def X6 : MyReg<"x6">;
+def X7 : MyReg<"x7">;
+def X8 : MyReg<"x8">;
+def X9 : MyReg<"x9">;
+def X10 : MyReg<"x10">;
+def X11 : MyReg<"x11">;
+def X12 : MyReg<"x12">;
+def X13 : MyReg<"x13">;
+def X14 : MyReg<"x14">;
+def X15 : MyReg<"x15">;
+
+def ValueModeVT : ValueTypeByHwMode<[DefaultMode, TestMode, TestMode1],
+ [i32, i64, f32]>;
+
+let RegInfos = RegInfoByHwMode<[DefaultMode, TestMode],
+ [RegInfo<32,32,32>, RegInfo<64,64,64>]> in
+def XRegs : MyClass<32, [ValueModeVT], (sequence "X%u", 0, 15)>;
+
+def sub_even : SubRegIndex<32> {
+ let SubRegRanges = SubRegRangeByHwMode<[DefaultMode, TestMode],
+ [SubRegRange<32>, SubRegRange<64>]>;
+}
+def sub_odd : SubRegIndex<32, 32> {
+ let SubRegRanges = SubRegRangeByHwMode<[DefaultMode, TestMode],
+ [SubRegRange<32, 32>, SubRegRange<64, 64>]>;
+}
+
+def XPairs : RegisterTuples<[sub_even, sub_odd],
+ [(decimate (rotl XRegs, 0), 2),
+ (decimate (rotl XRegs, 1), 2)]>;
+
+let RegInfos = RegInfoByHwMode<[DefaultMode, TestMode],
+ [RegInfo<64,64,32>, RegInfo<128,128,64>]> in
+def XPairsClass : MyClass<64, [untyped], (add XPairs)>;
+
+// Modes who are not controlling Register related features will be manipulated
+// the same as DefaultMode.
+// CHECK-REG-LABEL: RegisterClass XRegs:
+// CHECK-REG: SpillSize: { Default:32 TestMode:64 TestMode1:32 TestMode2:32 }
+// CHECK-REG: SpillAlignment: { Default:32 TestMode:64 TestMode1:32 TestMode2:32 }
+// CHECK-REG: Regs: X0 X1 X2 X3 X4 X5 X6 X7 X8 X9 X10 X11 X12 X13 X14 X15
+
+// CHECK-REG-LABEL: RegisterClass XPairsClass:
+// CHECK-REG: SpillSize: { Default:64 TestMode:128 TestMode1:64 TestMode2:64 }
+// CHECK-REG: SpillAlignment: { Default:32 TestMode:64 TestMode1:32 TestMode2:32 }
+// CHECK-REG: CoveredBySubRegs: 1
+// CHECK-REG: Regs: X0_X1 X2_X3 X4_X5 X6_X7 X8_X9 X10_X11 X12_X13 X14_X15
+
+// CHECK-REG-LABEL: SubRegIndex sub_even:
+// CHECK-REG: Offset: { Default:0 TestMode:0 TestMode1:0 TestMode2:0 }
+// CHECK-REG: Size: { Default:32 TestMode:64 TestMode1:32 TestMode2:32 }
+// CHECK-REG-LABEL: SubRegIndex sub_odd:
+// CHECK-REG: Offset: { Default:32 TestMode:64 TestMode1:32 TestMode2:32 }
+// CHECK-REG: Size: { Default:32 TestMode:64 TestMode1:32 TestMode2:32 }
+
+//============================================================================//
+//--------------------- Encoding/Decoding parts ------------------------------//
+//============================================================================//
+def fooTypeEncDefault : InstructionEncoding {
+ let Size = 8;
+ field bits<64> SoftFail = 0;
+ bits<64> Inst;
+ bits<8> factor;
+ let Inst{7...0} = factor;
+ let Inst{3...2} = 0b10;
+ let Inst{1...0} = 0b00;
+}
+
+def fooTypeEncA : InstructionEncoding {
+ let Size = 4;
+ field bits<32> SoftFail = 0;
+ bits<32> Inst;
+ bits<8> factor;
+ let Inst{7...0} = factor;
+ let Inst{3...2} = 0b11;
+ let Inst{1...0} = 0b00;
+}
+
+
+def foo : Instruction {
+ let OutOperandList = (outs);
+ let InOperandList = (ins i32imm:$factor);
+ let EncodingInfos = EncodingByHwMode<
+ [TestMode2, DefaultMode], [fooTypeEncA, fooTypeEncDefault]
+ >;
+ let AsmString = "foo $factor";
+}
+
+// CHECK-SUBTARGET-LABEL: unsigned TestTargetGenSubtargetInfo::getHwModeSet() const {
+// CHECK-SUBTARGET: unsigned Modes = 0;
+// CHECK-SUBTARGET: if (checkFeatures("+feat")) Modes |= (1 << 0);
+// CHECK-SUBTARGET: if (checkFeatures("+feat1")) Modes |= (1 << 1);
+// CHECK-SUBTARGET: if (checkFeatures("+feat2")) Modes |= (1 << 2);
+// CHECK-SUBTARGET: return Modes;
+// CHECK-SUBTARGET: }
+// CHECK-SUBTARGET-LABEL: unsigned TestTargetGenSubtargetInfo::getHwMode(enum HwModeType type) const {
+// CHECK-SUBTARGET: unsigned Modes = getHwModeSet();
+// CHECK-SUBTARGET: if(!Modes)
+// CHECK-SUBTARGET: return Modes;
+// CHECK-SUBTARGET: switch (type) {
+// CHECK-SUBTARGET: case HwMode_Default:
+// CHECK-SUBTARGET: return llvm::countr_zero(Modes) + 1;
+// CHECK-SUBTARGET: break;
+// CHECK-SUBTARGET: case HwMode_ValueType: {
+// CHECK-SUBTARGET: Modes &= 3;
+// CHECK-SUBTARGET: if (!Modes)
+// CHECK-SUBTARGET: return Modes;
+// CHECK-SUBTARGET: if (!llvm::has_single_bit<unsigned>(Modes))
+// CHECK-SUBTARGET: llvm_unreachable("Two or more HwModes for ValueType were found!");
+// CHECK-SUBTARGET: return llvm::countr_zero(Modes) + 1;
+// CHECK-SUBTARGET: }
+// CHECK-SUBTARGET: case HwMode_RegInfo: {
+// CHECK-SUBTARGET: Modes &= 1;
+// CHECK-SUBTARGET: if (!Modes)
+// CHECK-SUBTARGET: return Modes;
+// CHECK-SUBTARGET: if (!llvm::has_single_bit<unsigned>(Modes))
+// CHECK-SUBTARGET: llvm_unreachable("Two or more HwModes for RegInfo were found!");
+// CHECK-SUBTARGET: return llvm::countr_zero(Modes) + 1;
+// CHECK-SUBTARGET: }
+// CHECK-SUBTARGET: case HwMode_EncodingInfo: {
+// CHECK-SUBTARGET: Modes &= 4;
+// CHECK-SUBTARGET: if (!Modes)
+// CHECK-SUBTARGET: return Modes;
+// CHECK-SUBTARGET: if (!llvm::has_single_bit<unsigned>(Modes))
+// CHECK-SUBTARGET: llvm_unreachable("Two or more HwModes for Encoding were found!");
+// CHECK-SUBTARGET: return llvm::countr_zero(Modes) + 1;
+// CHECK-SUBTARGET: }
+// CHECK-SUBTARGET: }
+// CHECK-SUBTARGET: return 0; // should not get here
+// CHECK-SUBTARGET: }
+
diff --git a/llvm/test/TableGen/HwModeEncodeDecode3.td b/llvm/test/TableGen/HwModeEncodeDecode3.td
index 8e0266b2c55af9..bc65d4a1d40d3b 100644
--- a/llvm/test/TableGen/HwModeEncodeDecode3.td
+++ b/llvm/test/TableGen/HwModeEncodeDecode3.td
@@ -160,15 +160,22 @@ def unrelated: Instruction {
// DECODER-SUPPRESS-O2-DAG: Opcode: fooTypeEncA:baz
// DECODER-SUPPRESS-O2-NOT: Opcode: bar
-// ENCODER-LABEL: static const uint64_t InstBits_DefaultMode[] = {
+// For 'bar' and 'unrelated', we didn't assign any hwmodes for them,
+// they should keep the same in the following three tables.
+// For 'foo' we assigned three hwmodes(includes 'DefaultMode')
+// it's encodings should be different in the following three tables.
+// For 'baz' we only assigned ModeB for it, to avoid empty encoding
+// we assigned the encoding of ModeB to ModeA and DefaultMode(Even though
+// they will not be used).
+// ENCODER-LABEL: static const uint64_t InstBits[] = {
// ENCODER: UINT64_C(2), // bar
-// ENCODER: UINT64_C(0), // baz
+// ENCODER: UINT64_C(12), // baz
// ENCODER: UINT64_C(8), // foo
// ENCODER: UINT64_C(2), // unrelated
// ENCODER-LABEL: static const uint64_t InstBits_ModeA[] = {
// ENCODER: UINT64_C(2), // bar
-// ENCODER: UINT64_C(0), // baz
+// ENCODER: UINT64_C(12), // baz
// ENCODER: UINT64_C(12), // foo
// ENCODER: UINT64_C(2), // unrelated
@@ -178,18 +185,53 @@ def unrelated: Instruction {
// ENCODER: UINT64_C(3), // foo
// ENCODER: UINT64_C(2), // unrelated
-// ENCODER: unsigned HwMode = STI.getHwMode();
-// ENCODER: switch (HwMode) {
-// ENCODER: default: llvm_unreachable("Unknown hardware mode!"); break;
-// ENCODER: case 0: InstBits = InstBits_DefaultMode; break;
-// ENCODER: case 1: InstBits = InstBits_ModeA; break;
-// ENCODER: case 2: InstBits = InstBits_ModeB; break;
-// ENCODER: };
-
-// ENCODER: case ::foo: {
-// ENCODER: switch (HwMode) {
-// ENCODER: default: llvm_unreachable("Unhandled HwMode");
-// ENCODER: case 0: {
-// ENCODER: case 1: {
-// ENCODER: case 2: {
-
+// ENCODER-LABEL: case ::bar:
+// ENCODER-LABEL: case ::unrelated:
+// ENCODER-NOT: getHwMode
+// ENCODER-LABEL: case ::baz: {
+// ENCODER: unsigned HwMode = STI.getHwMode(MCSubtargetInfo::HwMode_EncodingInfo);
+// ENCODER: HwMode &= 2;
+// ENCODER: switch (HwMode) {
+// ENCODER: default: llvm_unreachable("Unknown hardware mode!"); break;
+// ENCODER: case 2: InstBitsByHw = InstBits_ModeB; break;
+// ENCODER: };
+// ENCODER: Value = InstBitsByHw[opcode];
+// ENCODER: switch (HwMode) {
+// ENCODER: default: llvm_unreachable("Unhandled HwMode");
+// ENCODER: case 2: {
+// ENCODER: op = getMachineOpValue(MI, MI.getOperand(0), Fixups, STI);
+// ENCODER: op &= UINT64_C(240);
+// ENCODER: Value |= op;
+// ENCODER: break;
+// ENCODER: }
+// ENCODER-LABEL: case ::foo: {
+// ENCODER: unsigned HwMode = STI.getHwMode(MCSubtargetInfo::HwMod...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
superZWT123
changed the title
superZWT123
force-pushed
the
feature_tablegen_hwmode_bitset_access
branch
from
53f655d to
a02945a
Compare
|
Many discussions have already taken place in the previous PR. This patch makes numerous iterative improvements based on previous discussions and includes a simple test case for implementing the EncodingByHwMode feature in the RISCV backend (for reference only, to be deleted eventually), please focus mainly on the first commit. I believe this modification will make HwMode more user-friendly and scalable. |
|
@wangpc-pp @nvjle @topperc @kparzysz , hi if any of you could help me review this PR, I would be greatly thankful~ |
| @@ -1150,15 +1150,21 @@ def Feature32Bit | |||
| : SubtargetFeature<"32bit", "IsRV32", "true", "Implements RV32">; | |||
| def Feature64Bit | |||
| : SubtargetFeature<"64bit", "IsRV64", "true", "Implements RV64">; | |||
| def FeatureEncodingTmp | |||
There was a problem hiding this comment.
What's this for? Testing?
There was a problem hiding this comment.
Yes, this is just a reference and will be deleted eventually. It’s mainly to help everyone easily understand the purpose of this patch.
|
Ping |
| @@ -11,22 +11,60 @@ | |||
| // | |||
| //===----------------------------------------------------------------------===// | |||
|
|
|||
| multiclass tmpEclass<bits<3> imm3> { | |||
There was a problem hiding this comment.
Please do not add temporary code to a real target. You should write a unit test for your change. There many in llvm/test/TableGen/ already.
There was a problem hiding this comment.
Sure, in this PR, only the first commit is intended for merging into the main branch. The second commit serves merely as a helpful reference for the review process and is not meant for merging. I have deleted it, but if you need a reference, you can check this link : simple-test. I provided a reference implementation on RISCV backend because I think that the testcases in the /TableGen directory may not adequately showcase the PR’s impact fully. Thank you.
superZWT123
force-pushed
the
feature_tablegen_hwmode_bitset_access
branch
from
a02945a to
c5f65ee
Compare
|
Though I have only briefly reviewed this (beyond what I did in the original series a few weeks ago), we did apply the patches locally to a production downstream back end. Unfortunately, we're getting a number of failures in both disassembler and encoding tests. Just a quick diff of the One immediate hunch is that our back end has long instruction words of a few different widths-- starting at 128 bits and only getting larger from there. This means that the decoder and encoder emitters switch to APInt mode, which may or may not have been carefully tested in your patch. No in-tree back ends have long words, but at least a few downstream ones do-- and I'm not sure if yours does. As an aside, it is difficult to avoid breakage in different downstream back ends for features that are not (yet) used in any in-tree back ends. Ours will be upstreamed eventually, but the timeline is unknown. Until then, it is possible changes like this in one or another downstream target will need to stay local (we have many). Also, I agree with Craig that the pipe-cleaning/test changes in the RISCV back end need to be removed. They also lengthen and obscure the core patch. The unit tests should cover all of that functionality. If possible, please also make sure the long-word APInt modes of the encoder (and anywhere else changes were made) are covered in the testing. It isn't possible, of course, to do actual disassembler/encoder tests since there is no in-tree back end, but at least you can sanity check the encoder emitter output for that mode (I didn't see it in the current tests). (Edited: I have since reviewed the code, see additional comments). |
| /// HwMode ID will be stored as bits, allowing users to pull the specific | ||
| /// HwMode ID (like RegInfo HwMode ID) from the bits as needed. This enables | ||
| /// users to control multiple features with one hwmode (as previously) or use | ||
| /// different hwmodes to control different features. |
There was a problem hiding this comment.
The language "stored as bits" here is a tad bit strange (either too vague or too general). I think it is better to describe this for what it now is-- a bit vector or bit set.
| // SubRegRange. | ||
| HwMode_EncodingInfo // Return the HwMode ID that controls the EncodingInfo. | ||
| }; | ||
|
|
There was a problem hiding this comment.
Since there are now two APIs, please document each one specifically and clearly. Minimally, something along the lines of "Returns a bit vector/set of all hardware modes." for the first, and "Returns a single hardware mode of the requested type."
|
|
||
| // ENCODER: case ::foo: { | ||
| // ENCODER: switch (HwMode) { | ||
| // ENCODER: default: llvm_unreachable("Unhandled HwMode"); |
There was a problem hiding this comment.
Please add one more hardware mode ModeC and use it in one of the EncodingInfos (or add a new unit test file altogether). The reason why will be clear later in this review where I discuss the emitter code that generates the above-- and it would only show if there is at least one more mode.
| // ENCODER: UINT64_C(8), // foo | ||
| // ENCODER: UINT64_C(2), // unrelated | ||
|
|
||
| // ENCODER-LABEL: static const uint64_t InstBits_ModeA[] = { | ||
| // ENCODER: UINT64_C(2), // bar | ||
| // ENCODER: UINT64_C(0), // baz | ||
| // ENCODER: UINT64_C(12), // baz |
There was a problem hiding this comment.
It doesn't make sense to me to assign the encoding of a different mode for these unused entries. Why don't we leave them as zeroes as they originally were? In theory it doesn't matter, but it seems confusing to assign a non-zero value from some other mode. Also, for downstream back ends with any failures, diff'ing the original and new emitted files will have fewer "irrelevant" differences.
| append(" case " + itostr(DefaultMode) + | ||
| ": InstBitsByHw = InstBits"); | ||
| } else { | ||
| append(" case " + itostr(1 << (ModeId - 1)) + |
There was a problem hiding this comment.
As mentioned earlier, I think there is a bug here with generating these case values. That is, the switch is on HwMode,which was assigned by the getHwMode() above it. Now as I understand it, getHwMode returns a HwMode integer value corresponding to a particular mode-- not a bit position in the bit vector. On the other hand, the case label is 1 << (ModeId - 1)-- a bit position. It seems that we're comparing apples and oranges. Your test case wouldn't show this because the expressions happen to be equal for 0, 1, and 2. If you add another mode, I think you'll find its case value to be 4, not 3.
That would certainly explain at least some of the failures we're seeing downstream where we will indeed see a ModeId larger than 2.
There was a problem hiding this comment.
Thank you very much, I believe this is currently the biggest issue. There is indeed a logical problem here that I will fix soon. It’s due to changes I made to the interface, which resulted in mishandling the logic here. Thank you very much!
| @@ -1781,13 +1781,68 @@ void SubtargetEmitter::EmitHwModeCheck(const std::string &ClassName, | |||
| if (CGH.getNumModeIds() == 1) | |||
| return; | |||
|
|
|||
| OS << "unsigned " << ClassName << "::getHwMode() const {\n"; | |||
| // Collect all HwModes and related features defined in the TD files, | |||
| // and store them in bit format. | |||
There was a problem hiding this comment.
Similarly to earlier comments: "...store them in bit format." -> "store them as a bit vector."
|
|
||
| // Start emitting for getHwModeSet(). | ||
| OS << "unsigned " << ClassName << "::getHwModeSet() const {\n"; | ||
| OS << " // Collect HwModes and store them in bits\n"; |
There was a problem hiding this comment.
Even though this is auto-generated code, we should try to adhere to the Coding Standards (i.e., comments are proper prose with punctuation, full stop, etc.).
| OS << "unsigned " << ClassName | ||
| << "::getHwMode(enum HwModeType type) const {\n"; | ||
| OS << " unsigned Modes = getHwModeSet();\n"; | ||
| OS << "\n if(!Modes)\n return Modes;\n\n"; |
There was a problem hiding this comment.
As above, CS nit: if(!Modes) -> if (!Modes)
| << " if (!llvm::has_single_bit<unsigned>(Modes))\n" | ||
| << " llvm_unreachable(\"Two or more HwModes for Encoding were " | ||
| "found!\");\n" | ||
| << " return llvm::countr_zero(Modes) + 1;\n }\n"; |
There was a problem hiding this comment.
All three of the non-Default cases are essentially identical other than the mask value. We should collapse this into ~1/3 the code.
| "found!\");\n" | ||
| << " return llvm::countr_zero(Modes) + 1;\n }\n"; | ||
| OS << " }\n"; | ||
| OS << " return 0; // should not get here\n}\n"; |
There was a problem hiding this comment.
Should this instead be llvm_unreachable("unexpected HwModeType")?
|
Hi @nvjle, Thank you very much for your review, I will respond as promptly as possible. My backend has 128-bit wide instructions, so my initial development was based on APInt mode. Later, the testing was switched to a 64-bit instruction backend, some issues were detected during the switching testing process, and I promptly fixed them. The functionality works correctly in both cases. These days, I am also quite busy as the company has many development tasks that need my attention, so I can only allocate some spare time to modify this patch. Nonetheless, I am eager to contribute to the community and help drive improvements in this module. As you mentioned, the changes in this patch are sensitive and have a wide impact, so there is no rush. I will proceed slowly to ensure the correctness of this feature. Your review comments are crucial in this process, so once again, thank you~ |
superZWT123
force-pushed
the
feature_tablegen_hwmode_bitset_access
branch
2 times, most recently
from
f544d3e to
8853d1c
Compare
1. Bitwise operations are used to access HwMode, allowing for the coexistence of HwMode IDs for different features (such as RegInfo and EncodingInfo). This will provide better scalability for HwMode. Currently, most users utilize HwMode primarily for configuring Register-related information, and few use it for configuring Encoding. The limited scalability of HwMode has been a significant factor in this usage pattern. 2. Sink the HwMode Encodings selection logic down to per instruction level, this makes the logic for choosing encodings clearer and provides better error messages. 3. Add some HwMode ID conflict detection to the getHwMode() interface.
1. Since we have obtained HwMode Id from getHwMode() interface, we no longer need to perform bit related operations when checking the case. 2. When generating the 'InstBits' table, if the instruction does not have the HwMode ID corresponding to the current hwmode table, the base encoding will be generated as '0'. 3. Add testcase to test the correctness of hwmode encoding in APInt mode 4. Fixed a bug in the 'getOperandBitOffset' interface: it would check the HwMode ID for a instruction but did not preemptively fetch the HwMode ID. 5. Fix up some comments and clean up some codes.
superZWT123
force-pushed
the
feature_tablegen_hwmode_bitset_access
branch
from
8853d1c to
edc0c27
Compare
|
Hi @nvjle , I'm really sorry for the delay in addressing your review feedback. This update includes the following five main changes:
Could you please review these changes again and verify them in your backend? Thank you very much! |
|
Hi @iii-i , I have discovered and resolved a potential issue where the existing 'getOperandBitOffset' interface was checking HwMode ID without first retrieving it. Could you please help me review the implementation of that part? Thank you very much! |
Please give me a few days to re-review this iteration and incorporate into our repo to verify failures are gone. |
There was a problem hiding this comment.
Thank you @superZWT123, all concerns have been addressed-- and this now passes when incorporated to our downstream back end.
LGTM, ready to land.
|
The |
Uh oh!
There was an error while loading. Please reload this page.