[PATCH] D85225: [Target][AArch64] Allow for char as int8_t in AArch64AsmParser.cpp

[Rainer Orth via Phabricator via llvm-commits]

ro added a comment.

We've got 3 alternative versions now:

- my original one as simplified by Paul's suggestion (hadn't uploaded that, but certainly could recreate/retest if that's the selected candidate) Diff 1 in the History
- the sizeof version, Diff 3 in the History
- the (partial) non-template version, Diff 4 in the history

I guess it's decision time now ;-)



================
Comment at: llvm/lib/Target/AArch64/MCTargetDesc/AArch64AddressingModes.h:769
 
   if (std::is_same<int16_t, std::make_signed_t<T>>::value)
     return IsImm8 || IsImm16 || uint16_t(Imm & ~0xff) == Imm;
----------------
jyknight wrote:
> paulwalker-arm wrote:
> > jyknight wrote:
> > > sizeof(T) == 2 for consistency?
> > sizeof(float16_t) == 2, which nicely highlights why I don't like the sizeof approach.  If not already, someday somebody we'll want to support a 1 byte type that isn't an integer.
> But nobody should call these functions with T not being an integral type in the first place. It wouldn't make sense today or in the future, for what these functions do.
> 
> Actually, I'm not sure why these are even functions templated on a integer type T, rather than just passing a second "int ImmBytes" argument. At first glance, that would actually seem clearer all around.
I've just uploaded a version that does that, tested on `sparcv9-sun-solaris2.11` and `x86_64-pc-linux-gnu`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85225