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

Sun Feb 9 16:56:21 PST 2020

andreisfr marked 3 inline comments as done and 2 inline comments as done.
andreisfr added inline comments.

================
Comment at: llvm/include/llvm/BinaryFormat/ELF.h:775
+  /* Various CPU types.  */
+  E_XTENSA_MACH = 0x00000000,
+  EF_XTENSA_XT_INSN = 0x00000100,
----------------
aykevl wrote:
> 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.
Changed  E_XTENSA_MACH to EF_XTENSA_MACH_BASE

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

https://reviews.llvm.org/D64827