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

[Rainer Orth via Phabricator via llvm-commits]

ro created this revision.
ro added reviewers: t.p.northover, ostannard, rengolin, c-rhodes.
Herald added subscribers: danielkiss, fedor.sergeev, hiraditya, kristof.beyls, jyknight.
Herald added a project: LLVM.
ro requested review of this revision.

A couple of AArch64 tests were failing on Solaris, both sparc and x86:

  LLVM :: MC/AArch64/SVE/add-diagnostics.s
  LLVM :: MC/AArch64/SVE/cpy-diagnostics.s
  LLVM :: MC/AArch64/SVE/cpy.s
  LLVM :: MC/AArch64/SVE/dup-diagnostics.s
  LLVM :: MC/AArch64/SVE/dup.s
  LLVM :: MC/AArch64/SVE/mov-diagnostics.s
  LLVM :: MC/AArch64/SVE/mov.s
  LLVM :: MC/AArch64/SVE/sqadd-diagnostics.s
  LLVM :: MC/AArch64/SVE/sqsub-diagnostics.s
  LLVM :: MC/AArch64/SVE/sub-diagnostics.s
  LLVM :: MC/AArch64/SVE/subr-diagnostics.s
  LLVM :: MC/AArch64/SVE/uqadd-diagnostics.s
  LLVM :: MC/AArch64/SVE/uqsub-diagnostics.s

For example, reduced from `MC/AArch64/SVE/add-diagnostics.s`:

  add     z0.b, z0.b, #0, lsl #8

missed the expected diagnostics

  $ ./bin/llvm-mc -triple=aarch64 -show-encoding -mattr=+sve add.s
  add.s:1:21: error: immediate must be an integer in range [0, 255] with a shift amount of 0
  add     z0.b, z0.b, #0, lsl #8
                      ^

The message is `Match_InvalidSVEAddSubImm8`, emitted in the generated
`lib/Target/AArch64/AArch64GenAsmMatcher.inc` for `MCK_SVEAddSubImm8`.
When comparing the call to `::AArch64Operand::isSVEAddSubImm<char>` on both
Linux/x86_64 and Solaris, I find

  875	    bool IsByte = std::is_same<int8_t, std::make_signed_t<T>>::value;

is `false` on Solaris, unlike Linux.

The problem boils down to the fact that `int8_t` is plain `char` on Solaris: both the sparc
and i386 psABIs have `char` as signed.  However, with

  9887	    DiagnosticPredicate DP(Operand.isSVEAddSubImm<int8_t>());

in `lib/Target/AArch64/AArch64GenAsmMatcher.inc`, `std::make_signed_t<int8_t>`
above yieds `signed char`, so `std::is_same<int8_t, char>` is `false`.

This can easily be fixed by also allowing for `signed char` here and in a few similar places.

Tested on `amd64-pc-solaris2.11`, `sparcv9-sun-solaris2.11`, and `x86_64-pc-linux-gnu`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D85225

Files:
  llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
  llvm/lib/Target/AArch64/MCTargetDesc/AArch64AddressingModes.h


Index: llvm/lib/Target/AArch64/MCTargetDesc/AArch64AddressingModes.h
===================================================================
--- llvm/lib/Target/AArch64/MCTargetDesc/AArch64AddressingModes.h
+++ llvm/lib/Target/AArch64/MCTargetDesc/AArch64AddressingModes.h
@@ -763,7 +763,8 @@
   bool IsImm8 = int8_t(Imm) == Imm;
   bool IsImm16 = int16_t(Imm & ~0xff) == Imm;
 
-  if (std::is_same<int8_t, std::make_signed_t<T>>::value)
+  if (std::is_same<int8_t, std::make_signed_t<T>>::value ||
+      std::is_same<signed char, std::make_signed_t<T>>::value)
     return IsImm8 || uint8_t(Imm) == Imm;
 
   if (std::is_same<int16_t, std::make_signed_t<T>>::value)
@@ -775,7 +776,8 @@
 /// Returns true if Imm is valid for ADD/SUB.
 template <typename T>
 static inline bool isSVEAddSubImm(int64_t Imm) {
-  bool IsInt8t = std::is_same<int8_t, std::make_signed_t<T>>::value;
+  bool IsInt8t = std::is_same<int8_t, std::make_signed_t<T>>::value ||
+                 std::is_same<signed char, std::make_signed_t<T>>::value;
   return uint8_t(Imm) == Imm || (!IsInt8t && uint16_t(Imm & ~0xff) == Imm);
 }
 
Index: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
===================================================================
--- llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
+++ llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
@@ -855,7 +855,8 @@
     if (!isShiftedImm() && (!isImm() || !isa<MCConstantExpr>(getImm())))
       return DiagnosticPredicateTy::NoMatch;
 
-    bool IsByte = std::is_same<int8_t, std::make_signed_t<T>>::value;
+    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>())
       if (!(IsByte && ShiftedImm->second) &&
           AArch64_AM::isSVECpyImm<T>(uint64_t(ShiftedImm->first)
@@ -872,7 +873,8 @@
     if (!isShiftedImm() && (!isImm() || !isa<MCConstantExpr>(getImm())))
       return DiagnosticPredicateTy::NoMatch;
 
-    bool IsByte = std::is_same<int8_t, std::make_signed_t<T>>::value;
+    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>())
       if (!(IsByte && ShiftedImm->second) &&
           AArch64_AM::isSVEAddSubImm<T>(ShiftedImm->first


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D85225.282911.patch
Type: text/x-patch
Size: 2386 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200804/376f6b3b/attachment.bin>