RE: [PATCH v3 06/44] gdb, arch, intelgt: add intelgt arch definitions

["Metzger, Markus T" <markus.t.metzger@intel.com>]

Hello Simon,

Thanks for your review.

>> +/* Various arch constants.  */
>> +
>> +enum breakpoint_kind
>> +{
>> +  BP_INSTRUCTION = 1,
>> +};
>
>enum class, for new stuff?  You can get rid of the prefix then.

The only use is in breakpoint_kind_from_pc(), where it needs to decay
to int.

Similarly, the dwarf_regset enum below is used as array indices.

The only candidate for an enum class is xe_version.

>> +
>> +/* The length of a full and compact IntelGT instruction in bytes.  */
>> +
>> +constexpr int MAX_INST_LENGTH = 16;
>> +constexpr int COMPACT_INST_LENGTH = 8;
>> +
>> +/* Feature names.
>> +
>> +   They correspond to register sets defined in zet_intel_gpu_debug.h.  We
>> +   declare feature names in the order used in that header.
>> +
>> +   The SBA register set consists of a set of base registers in the order
>> +   defined in that header file.
>> +
>> +   Not all registers have DWARF numbers.  See DWARF_REGSETS below for a
>> +   list of features that do.  */
>> +constexpr const char *FEATURE_GRF = "org.gnu.gdb.intelgt.grf";
>> +constexpr const char *FEATURE_ADDR = "org.gnu.gdb.intelgt.addr";
>> +constexpr const char *FEATURE_FLAG = "org.gnu.gdb.intelgt.flag";
>> +constexpr const char *FEATURE_CE = "org.gnu.gdb.intelgt.ce";
>> +constexpr const char *FEATURE_SR = "org.gnu.gdb.intelgt.sr";
>> +constexpr const char *FEATURE_CR = "org.gnu.gdb.intelgt.cr";
>> +constexpr const char *FEATURE_TDR = "org.gnu.gdb.intelgt.tdr";
>> +constexpr const char *FEATURE_ACC = "org.gnu.gdb.intelgt.acc";
>> +constexpr const char *FEATURE_MME = "org.gnu.gdb.intelgt.mme";
>> +constexpr const char *FEATURE_SP = "org.gnu.gdb.intelgt.sp";
>> +constexpr const char *FEATURE_SBA = "org.gnu.gdb.intelgt.sba";
>> +constexpr const char *FEATURE_DBG = "org.gnu.gdb.intelgt.dbg";
>> +constexpr const char *FEATURE_FC = "org.gnu.gdb.intelgt.fc";
>> +constexpr const char *FEATURE_DEBUGGER =
>"org.gnu.gdb.intelgt.debugger";
>> +
>> +/* Register sets/groups needed for DWARF mapping.  Used for
>> +   declaring static arrays for various mapping tables.  */
>> +
>> +enum dwarf_regsets : int
>> +{
>> +  REGSET_SBA = 0,
>> +  REGSET_GRF,
>> +  REGSET_ADDR,
>> +  REGSET_FLAG,
>> +  REGSET_ACC,
>> +  REGSET_MME,
>> +  REGSET_COUNT
>> +};
>> +
>> +/* Map of dwarf_regset values to the target description
>> +   feature names.  */
>> +
>> +constexpr const char *DWARF_REGSET_FEATURES[REGSET_COUNT] = {
>> +  FEATURE_SBA,
>> +  FEATURE_GRF,
>> +  FEATURE_ADDR,
>> +  FEATURE_FLAG,
>> +  FEATURE_ACC,
>> +  FEATURE_MME
>> +};
>> +
>> +/* The encoding for XE version enumerates follows this pattern, which is
>
>enumerates -> enumerators?
>
>> +   aligned with the IGA encoding.  */
>
>What is IGA?  Around here it's a grocery store (https://www.iga.net). :)

It's Intel Graphics (dis)Assembler.  I spelled it out in the comment.

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Dornacher Straße 1, 85622 Feldkirchen, Germany
Tel: +49 89 991 430, www.intel.de
Managing Directors: Harry Demas, Jeffrey Schneiderman, Yin Chong Sorrell
Chairperson of the Supervisory Board: Nicole Lau
Registered Seat: Munich
Commercial Register: Amtsgericht München HRB 186928