[clang][MIPS] Removing __SIZEOF_INT128__ macro for MIPS64

[slthakur]

Summary

SIZEOF_INT128 macro should not be defined as 128-bit operations are not yet supported for MIPS64.

Failing tests:
  Lexer/ms-extensions.c:

Test assumes if SIZEOF_INT128 is not defined then use of suffix i128 should throw an error message "int128 is not supported on this target". In MIPS, declaration of int128 type is necessary even though SIZEOF_INT128 is undefined because standard header files like limits.h throw error message if __int128 is not available.

[atanasyan]

Could you add a test case checks that the __SIZEOF_INT128__ macro is not defined?

[slthakur]

The previous was not allowing declaration of int128 types in standard
header files like limit.h.
This patch will allow declaration of int128 type and just undefined the 128-bit macro
for mips64 and mips64el.

[atanasyan]

LGTM with the only nit.

lib/Frontend/InitPreprocessor.cpp

665 ↗

(On Diff #16715)

I would add a comment describes that it is a temporary workaround while MIPS has no fully supported 128-bit integers and we do not want to get an error from limit.h.

[slthakur]

lib/Frontend/InitPreprocessor.cpp

665 ↗

(On Diff #16715)

Hi @atanasyan,

Should I add a comment and submit a new patch ?

[atanasyan]

lib/Frontend/InitPreprocessor.cpp

665 ↗

(On Diff #16715)

I think it is not necessary now, because this patch has been committed already.

[slthakur]

lib/Frontend/InitPreprocessor.cpp

665 ↗

(On Diff #16715)

Hi @atanasyan,

I don't see this patch commited in the clang repository ?

[atanasyan]

lib/Frontend/InitPreprocessor.cpp

665 ↗

(On Diff #16715)

My bad, I interpreted your question incorrectly. Add the comment to the patch and commit it. It is not necessary to send the patch for review again.

[majnemer]

lib/Frontend/InitPreprocessor.cpp

665 ↗

(On Diff #16715)

Wait, why not just override Mips64TargetInfoBase:: hasInt128Type to be false?

[slthakur]

lib/Frontend/InitPreprocessor.cpp

665 ↗

(On Diff #16715)

@majnemer : When we override Mips64TargetInfoBase:: hasInt128Type to be false, we get an error message "int128 is not supported on this target" from standard header files like limits.h because these files are using __int128 type and suffix i128. Here, we only want to remove __SIZEOF_INT128__ macro for mips64 and misp64el because 128-bit operations are not yet supported on MIPS64. This change is temporary, until we implement 128-bit operations for MIPS64.

[slthakur]

Added comment

[slthakur]

Hi @atanasyan,

I don't have commit access. Can you commit it for me ?

[majnemer]

I was able to #include <limits.h> with just the following diff:

diff --git a/lib/Basic/Targets.cpp b/lib/Basic/Targets.cpp
index b28808b..67bff45 100644

• a/lib/Basic/Targets.cpp

+++ b/lib/Basic/Targets.cpp
@@ -5971,7 +5971,7 @@ public:

  NumAliases = llvm::array_lengthof(GCCRegAliases);
}

• bool hasInt128Type() const override { return true; }

+ bool hasInt128Type() const override { return false; }
};

class Mips64EBTargetInfo : public Mips64TargetInfoBase {

I tested it with: echo '#include <limits.h>' |
~/llvm/Debug+Asserts/bin/clang -x c -target mips64-none-none - -fsyntax-only

[slthakur]

Hi @majnemer,

Sorry, my mistake. The problem arises when we #include c++ header file <limits> and not with c header file <limits.h>

I tested it with: echo '#include <limits>' | clang++ -x c++ -target mips64-linux-gnu - -fsyntax-only.

I will change the comment.

[slthakur]

Changed comment in /lib/Frontend/InitPreprocessor.cpp

[dsanders]

Hi,

I stumbled across the remaining part of the backend problem while investigating some failures on a mips64 host and mips64-linux-gnu triple. http://reviews.llvm.org/D6610 should fix it.

cfe/trunk/lib/Frontend/InitPreprocessor.cpp

#undef TOSTR2
   DefineTypeSizeof("__SIZEOF_PTRDIFF_T__",
                    TI.getTypeWidth(TI.getPtrDiffType(0)), TI, Builder);
   DefineTypeSizeof("__SIZEOF_SIZE_T__",
                    TI.getTypeWidth(TI.getSizeType()), TI, Builder);
   DefineTypeSizeof("__SIZEOF_WCHAR_T__",
                    TI.getTypeWidth(TI.getWCharType()), TI, Builder);
   DefineTypeSizeof("__SIZEOF_WINT_T__",
                    TI.getTypeWidth(TI.getWIntType()), TI, Builder);
-  if (TI.hasInt128Type())
+  // This is a temporary workaround while MIPS64 has not yet fully supported
+  // 128-bit integers. But declaration of int128 type is necessary even though
+  // __SIZEOF_INT128__ is undefined because c++ standard header files like
+  // limits throw error message if __int128 is not available.
+  if (TI.hasInt128Type() && !(TI.getTriple().getArch() == llvm::Triple::mips64el
+                   || TI.getTriple().getArch() == llvm::Triple::mips64))
     DefineTypeSizeof("__SIZEOF_INT128__", 128, TI, Builder);
 
   DefineType("__INTMAX_TYPE__", TI.getIntMaxType(), Builder);
   DefineFmt("__INTMAX", TI.getIntMaxType(), TI, Builder);
   Builder.defineMacro("__INTMAX_C_SUFFIX__",
                       TI.getTypeConstantSuffix(TI.getIntMaxType()));
   DefineType("__UINTMAX_TYPE__", TI.getUIntMaxType(), Builder);
   DefineFmt("__UINTMAX", TI.getUIntMaxType(), TI, Builder);

cfe/trunk/test/Preprocessor/init.c

 // MIPS64BE:#define __PTRDIFF_WIDTH__ 64
 // MIPS64BE:#define __REGISTER_PREFIX__ 
 // MIPS64BE:#define __SCHAR_MAX__ 127
 // MIPS64BE:#define __SHRT_MAX__ 32767
 // MIPS64BE:#define __SIG_ATOMIC_MAX__ 2147483647
 // MIPS64BE:#define __SIG_ATOMIC_WIDTH__ 32
 // MIPS64BE:#define __SIZEOF_DOUBLE__ 8
 // MIPS64BE:#define __SIZEOF_FLOAT__ 4
+// MIPS64BE-NOT:#define __SIZEOF_INT128__ 16
 // MIPS64BE:#define __SIZEOF_INT__ 4
 // MIPS64BE:#define __SIZEOF_LONG_DOUBLE__ 16
 // MIPS64BE:#define __SIZEOF_LONG_LONG__ 8
 // MIPS64BE:#define __SIZEOF_LONG__ 8
 // MIPS64BE:#define __SIZEOF_POINTER__ 8
 // MIPS64BE:#define __SIZEOF_PTRDIFF_T__ 8
 // MIPS64BE:#define __SIZEOF_SHORT__ 2
 // MIPS64BE:#define __SIZEOF_SIZE_T__ 8

 // MIPS64EL:#define __PTRDIFF_WIDTH__ 64
 // MIPS64EL:#define __REGISTER_PREFIX__ 
 // MIPS64EL:#define __SCHAR_MAX__ 127
 // MIPS64EL:#define __SHRT_MAX__ 32767
 // MIPS64EL:#define __SIG_ATOMIC_MAX__ 2147483647
 // MIPS64EL:#define __SIG_ATOMIC_WIDTH__ 32
 // MIPS64EL:#define __SIZEOF_DOUBLE__ 8
 // MIPS64EL:#define __SIZEOF_FLOAT__ 4
+// MIPS64EL-NOT:#define __SIZEOF_INT128__ 16
 // MIPS64EL:#define __SIZEOF_INT__ 4
 // MIPS64EL:#define __SIZEOF_LONG_DOUBLE__ 16
 // MIPS64EL:#define __SIZEOF_LONG_LONG__ 8
 // MIPS64EL:#define __SIZEOF_LONG__ 8
 // MIPS64EL:#define __SIZEOF_POINTER__ 8
 // MIPS64EL:#define __SIZEOF_PTRDIFF_T__ 8
 // MIPS64EL:#define __SIZEOF_SHORT__ 2
 // MIPS64EL:#define __SIZEOF_SIZE_T__ 8