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

[ro]

Summary

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.

[ro]

Ping? It's been a week.

[ro]

Ping^2? Two weeks now.

[paulwalker-arm]

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

859

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?

[ro]

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

859

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?

[paulwalker-arm]

I have no real objections to this patch other than I prefer:

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

if that works for your use case, if not then I'm happy with your current solution. Please can you give others a day or so longer to intervene in case they have a better suggestion.

[jyknight]

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

859

+1 to sizeof(T) == 1, that's a lot better.

[ro]

Just check sizeof(T) == 1.

[ro]

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

859

+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).

[jyknight]

llvm/lib/Target/AArch64/MCTargetDesc/AArch64AddressingModes.h

770

sizeof(T) == 2 for consistency?

[MaskRay]

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 ...

So, is -funsigned-char unsupported on Solaris?

[paulwalker-arm]

If the consensus is to accept sizeof(T)==1 then so be it, but personally it doesn't sit well with me.

llvm/lib/Target/AArch64/MCTargetDesc/AArch64AddressingModes.h

770

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.

[ro]

Use sizeof(T) == 2 check, too.
Update variable name.

[ro]

llvm/lib/Target/AArch64/MCTargetDesc/AArch64AddressingModes.h

781

Renamed IsInt8t: it no longer matches the check and according to my testing signedness didn't matter here.

[ro]

In D85225#2224935, @MaskRay wrote:

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 ...

So, is -funsigned-char unsupported on Solaris?

Depends on your definition of "supported", I'd say. The option does exactly what it says. If you only apply it to your own code without interacting with system code (libc etc.), you should be ok. However, as with other ABI-violating options, once you start using system code which by definition must follow the ABI, all bets are off.

Studio 12.6 cc(1) puts it nicely for its equivalent -xchar option:

The -xchar option changes the range of values for the type char only for code compiled with -xchar. This option does not change the range of values for type char in any system routine or header file. In particular, the value of CHAR_MAX and CHAR_MIN, as defined by limits.h, do not change when this option is specified. Therefore, CHAR_MAX and CHAR_MIN no longer represent the range of values encodable in a plain char.

If you use -xchar, be particularly careful when you compare a char against a predefined system macro because the value in the macro may be signed. This is most common for any routine that returns an error code which is accessed through a macro. Error codes are typically negative values so when you compare a char against the value from such a macro, the result is always false. A negative number can never be equal to any value of an unsigned type.

It is strongly recommended that you never use -xchar to compile routines for any interface exported through a library. The Oracle Solaris ABI specifies type char as signed, and system libraries behave accordingly. The effect of making char unsigned has not been extensively tested with system libraries. Instead of using this option, modify your code that it does not depend on whether type char is signed or unsigned. The sign of type char varies among compilers and operating systems.

[ro]

In D85225#2223849, @paulwalker-arm wrote:

I have no real objections to this patch other than I prefer:

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

if that works for your use case, if not then I'm happy with your current solution. Please can you give others a day or so longer to intervene in case they have a better suggestion.

I've now tested that variant, too, as before: works just as well.

I'm fine with either solution (although I prefer the sizeof one since it's way shorter/easier to read).

Just let me know when you've reached consensus.

[jyknight]

llvm/lib/Target/AArch64/MCTargetDesc/AArch64AddressingModes.h

770

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.

[ro]

Non-template version, passing ImmBytes around.

[ro]

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 ;-)

llvm/lib/Target/AArch64/MCTargetDesc/AArch64AddressingModes.h

770

I've just uploaded a version that does that, tested on sparcv9-sun-solaris2.11 and x86_64-pc-linux-gnu.

[paulwalker-arm]

My vote hasn't changed, I mean I already accepted the tweaked Option1 so you could have been done and dusted by now :) I just don't see what's gained from Option3's refactoring. It does the same as Option1 but in a less type safe/flexible/c++ way. Furthermore, the new interface is needlessly different to the other functions that handle SVE immediate values when the vector element type plays a role.

[paulwalker-arm]

@ro Is it worth specifying an end date for votes/opinions?

[ro]

In D85225#2244825, @paulwalker-arm wrote:

My vote hasn't changed, I mean I already accepted the tweaked Option1 so you could have been done and dusted by now :) I just don't see what's gained from Option3's refactoring. It does the same as Option1 but in a less type safe/flexible/c++ way. Furthermore, the new interface is needlessly different to the other functions that handle SVE immediate values when the vector element type plays a role.

When differerent reviewers expressed different preferences, I guess I expected they would reach some agreement among themselves. But I'm certainly not keen on dragging this along much longer, let alone try yet another variant ;-) Not being much of a C++ programmer, I can't say much one way or another myself. So I guess I'll give it another day or two for others to chime in and then commit the tweaked Option 1 to finally be done with this.

[jyknight]

ro: In this case, I would say to defer to Paul, although I wish that he would change his mind.

[ro]

Actual Option 1 variant.

[ro]

In D85225#2245621, @jyknight wrote:

ro: In this case, I would say to defer to Paul, although I wish that he would change his mind.

Ok, I'll commit the just attached version once testing to guard against stupid typos has completed.

Thanks.

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

public:
   // range -32768 to +32512.
   // For element-width of 8 bits a range of -128 to 255 is accepted,
   // since a copy of a byte can be either signed/unsigned.
   template <typename T>
   DiagnosticPredicate isSVECpyImm() const {
     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<int8_t, T>::value;
     if (auto ShiftedImm = getShiftedVal<8>())
       if (!(IsByte && ShiftedImm->second) &&
           AArch64_AM::isSVECpyImm<T>(uint64_t(ShiftedImm->first)
                                      << ShiftedImm->second))
         return DiagnosticPredicateTy::Match;
 
     return DiagnosticPredicateTy::NearMatch;
   }
 
   // Unsigned value in the range 0 to 255. For element widths of
   // 16 bits or higher it may also be a signed multiple of 256 in the
   // range 0 to 65280.
   template <typename T> DiagnosticPredicate isSVEAddSubImm() const {
     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<int8_t, T>::value;
     if (auto ShiftedImm = getShiftedVal<8>())
       if (!(IsByte && ShiftedImm->second) &&
           AArch64_AM::isSVEAddSubImm<T>(ShiftedImm->first
                                         << ShiftedImm->second))
         return DiagnosticPredicateTy::Match;
 
     return DiagnosticPredicateTy::NearMatch;
   }

llvm/lib/Target/AArch64/MCTargetDesc/AArch64AddressingModes.h

 }
 
 /// Returns true if Imm is valid for CPY/DUP.
 template <typename T>
 static inline bool isSVECpyImm(int64_t Imm) {
   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<int8_t, T>::value)
     return IsImm8 || uint8_t(Imm) == Imm;
 
   if (std::is_same<int16_t, std::make_signed_t<T>>::value)
     return IsImm8 || IsImm16 || uint16_t(Imm & ~0xff) == Imm;
 
   return IsImm8 || IsImm16;
 }
 
 /// 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<int8_t, T>::value;
   return uint8_t(Imm) == Imm || (!IsInt8t && uint16_t(Imm & ~0xff) == Imm);
 }
 
 /// Return true if Imm is valid for DUPM and has no single CPY/DUP equivalent.
 static inline bool isSVEMoveMaskPreferredLogicalImmediate(int64_t Imm) {
   if (isSVECpyImm<int64_t>(Imm))
     return false;
 
   auto S = bit_cast<std::array<int32_t, 2>>(Imm);