[PATCH] D64827: [Xtensa 2/10] Add Xtensa ELF definitions.

Sat Aug 17 17:10:19 PDT 2019

aykevl added a comment.

LGTM, but I know very little about the LLVM backend.
Also, I see that this patch lacks one change that are included by the very similar RISC-V patch: https://github.com/lowRISC/riscv-llvm/blob/master/0003-RISCV-Add-RISC-V-ELF-defines.patch, namely the equivalent of ElfHeaderRISCVFlags in ELFDumper.cpp. I don't know whether that has a reason but mentioning it just in case.

================
Comment at: llvm/include/llvm/BinaryFormat/ELF.h:775
+  /* Various CPU types.  */
+  E_XTENSA_MACH = 0x00000000,
+  EF_XTENSA_XT_INSN = 0x00000100,
----------------
andreisfr wrote:
> appcs wrote:
> > Typo? (Just making sure it isn't).
> I took as a basis naming conventions of the e_flags from binutils (binutils/include/elf/xtensa.h). There used similar names for the architecture mask and  base machine architecture with only differenece in prefix (E_ or EF_).
Looking at https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=include/elf/msp430.h for comparison, they use `EF_<arch>_MACH` for the mask and `E_<arch>_MACH_<sub>` for what seems to be the sub-architecture. I see a very similar pattern for AVR. In both cases, LLVM still uses the `EF_` prefix for the subarch.
For consistency with the other architectures in LLVM, it might be better to use something like `EF_XTENSA_MACH_BASE`. However, I have no idea why these constants are given these names in LLVM or binutils.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64827/new/

https://reviews.llvm.org/D64827