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

Rainer Orth via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Aug 18 14:09:21 PDT 2020


ro added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:859
+    bool IsByte = std::is_same<int8_t, std::make_signed_t<T>>::value ||
+                  std::is_same<signed char, std::make_signed_t<T>>::value;
     if (auto ShiftedImm = getShiftedVal<8>())
----------------
jyknight wrote:
> ro wrote:
> > paulwalker-arm wrote:
> > > Perhaps a naive question but are there any concerns about "signed char" being bigger than a byte on any systems?
> > > 
> > > The commit message suggests its the make_signed that is the problem so I'm wondering if instead adding "std::is_same<int8_t, T>::value" would also work?
> > > Perhaps a naive question but are there any concerns about "signed char" being bigger than a byte on any systems?
> > 
> > There shouldn't be: `sizeof(char/unsigned char/signed char)` is guaranteed to be `1` by C11, 6.5.3.4.
> > 
> > > The commit message suggests its the make_signed that is the problem so I'm wondering if instead adding "std::is_same<int8_t, T>::value" would also work?
> > 
> > At least if have found no case where `T` wasn't a signed type.  OTOH maybe s simple `sizeof(T) == 1` is enough?
> +1 to sizeof(T) == 1, that's a lot better.
> +1 to sizeof(T) == 1, that's a lot better.

Fully agreed.  The updated patch has been tested on `x86_64-pc-linux-gnu` (2-stage build), `sparcv9-sun-solaris2.11`, and `amd64-pc-linux-gnu` (1-stage build with gcc 10).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85225



More information about the llvm-commits mailing list