Separate builtins for x84-64 and i386; implement __mulh and __umulh

[agutowski]

Summary

We need x86-64-specific builtins if we want to implement some of the MS intrinsics - winnt.h contains definitions of some functions for i386, but not for x86-64 (for example _InterlockedOr64), which means that we cannot treat them as builtins for both i386 and x86-64, because then we have definitions of builtin functions in winnt.h on i386.

[agutowski]

include/clang/Basic/BuiltinsX86_32.def

1 ↗

(On Diff #71470)

Will we ever need that file? It seems to nicely complement the 64-bit one, but if it's going to be empty forever maybe it's better to remove it.

[rnk]

lib/Basic/Targets.cpp

2321

I'd rather not duplicate this readonly data. I had this clever idea that we'd do something like:

const Builtin::Info BuiltinInfo[] = {
...
#include "BuiltinsX86_32.def"
#include "BuiltinsX86.def"
#include "BuiltinsX86_64.def"
};

And then our makeArrayRef call would take the appropriate parts.

[rnk]

include/clang/Basic/BuiltinsX86_32.def

1 ↗

(On Diff #71470)

I guess we should remove it. Take a look at isX86_64Builtin in Sema/SemaChecking.cpp. It might be good to, as a follow-up change, move those builtins over to BuiltinsX86_64.def.

[agutowski]

remove BuiltinsX86_32.def; reduce redundancy in x86 builtins info

[rnk]

Let's avoid the duplicate enum, otherwise looks good

include/clang/Basic/TargetBuiltins.h

100

I think this would be better with just one enum to reduce compilation time:

  /// \brief X86 builtins
  namespace X86 {
  enum {
    LastTIBuiltin = clang::Builtin::FirstTSBuiltin - 1,
#define BUILTIN(ID, TYPE, ATTRS) BI##ID,
#include "clang/Basic/BuiltinsX86.def"
    FirstX86_64Builtin,
    LastX86CommonBuiltin = FirstX86_64Builtin - 1,
#define BUILTIN(ID, TYPE, ATTRS) BI##ID,
#include "clang/Basic/BuiltinsX86_64.def"
    LastTSBuiltin
  };
  }

[agutowski]

merge enums

[agutowski]

include/clang/Basic/TargetBuiltins.h

100

Nice, thanks!
As far as I see, it creates some inconsistency in usage of the word "last", because it's used wrong everywhere else - LastTSBuiltin is the number of the last target-specific builtin plus one, while LastX86CommonBuiltin doesn't have this additional one. I would leave it like that, but wanted to point that out, in case you think it's not acceptable.

[agutowski]

include/clang/Basic/TargetBuiltins.h

100

Ah, now I see that LastTIBuiltin is also what its name suggests, so it's only LastTSBuiltin that's different.

[rnk]

lgtm

include/clang/Basic/BuiltinsX86_64.def

+//===--- BuiltinsX86_64.def - X86-64 Builtin function database --*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines the X86-64-specific builtin function database. Users of
+// this file must define the BUILTIN macro to make use of this information.
+//
+//===----------------------------------------------------------------------===//
+
+// The format of this database matches clang/Basic/Builtins.def.
+
+#if defined(BUILTIN) && !defined(TARGET_HEADER_BUILTIN)
+#  define TARGET_HEADER_BUILTIN(ID, TYPE, ATTRS, HEADER, LANG, FEATURE) BUILTIN(ID, TYPE, ATTRS)
+#endif
+
+TARGET_HEADER_BUILTIN(__mulh,  "LLiLLiLLi",    "nch", "intrin.h", ALL_MS_LANGUAGES, "")
+TARGET_HEADER_BUILTIN(__umulh, "ULLiULLiULLi", "nch", "intrin.h", ALL_MS_LANGUAGES, "")
+
+#undef BUILTIN
+#undef TARGET_HEADER_BUILTIN

include/clang/Basic/TargetBuiltins.h

 
   /// \brief X86 builtins
   namespace X86 {
-    enum {
-        LastTIBuiltin = clang::Builtin::FirstTSBuiltin-1,
+    LastTIBuiltin = clang::Builtin::FirstTSBuiltin - 1,
 #define BUILTIN(ID, TYPE, ATTRS) BI##ID,
 #include "clang/Basic/BuiltinsX86.def"
-        LastTSBuiltin
-    };
+    FirstX86_64Builtin,
+    LastX86CommonBuiltin = FirstX86_64Builtin - 1,
+#define BUILTIN(ID, TYPE, ATTRS) BI##ID,
+#include "clang/Basic/BuiltinsX86_64.def"
+    LastTSBuiltin
+  };
   }
 
   /// \brief Flags to identify the types for overloaded Neon builtins.

lib/Basic/Targets.cpp

   return TargetInfo::initFeatureMap(Features, Diags, CPU, FeatureVec);
 }
 
-// Namespace for x86 abstract base class
-const Builtin::Info BuiltinInfo[] = {
+const Builtin::Info BuiltinInfoX86[] = {
 #define BUILTIN(ID, TYPE, ATTRS)                                               \
   { #ID, TYPE, ATTRS, nullptr, ALL_LANGUAGES, nullptr },
-#define LIBBUILTIN(ID, TYPE, ATTRS, HEADER)                                    \
-  { #ID, TYPE, ATTRS, HEADER, ALL_LANGUAGES, nullptr },
 #define TARGET_BUILTIN(ID, TYPE, ATTRS, FEATURE)                               \
   { #ID, TYPE, ATTRS, nullptr, ALL_LANGUAGES, FEATURE },
 #define TARGET_HEADER_BUILTIN(ID, TYPE, ATTRS, HEADER, LANGS, FEATURE)         \
   { #ID, TYPE, ATTRS, HEADER, LANGS, FEATURE },
 #include "clang/Basic/BuiltinsX86.def"
+
+#define BUILTIN(ID, TYPE, ATTRS)                                               \
+  { #ID, TYPE, ATTRS, nullptr, ALL_LANGUAGES, nullptr },
+#define TARGET_HEADER_BUILTIN(ID, TYPE, ATTRS, HEADER, LANGS, FEATURE)         \
+  { #ID, TYPE, ATTRS, HEADER, LANGS, FEATURE },
+#include "clang/Basic/BuiltinsX86_64.def"
 };
 
+
 static const char* const GCCRegNames[] = {
   "ax", "dx", "cx", "bx", "si", "di", "bp", "sp",
   "st", "st(1)", "st(2)", "st(3)", "st(4)", "st(5)", "st(6)", "st(7)",

     // X87 evaluates with 80 bits "long double" precision.
     return SSELevel == NoSSE ? 2 : 0;
   }
-  ArrayRef<Builtin::Info> getTargetBuiltins() const override {
-    return llvm::makeArrayRef(BuiltinInfo,
-                             clang::X86::LastTSBuiltin-Builtin::FirstTSBuiltin);
-  }
   ArrayRef<const char *> getGCCRegNames() const override {
     return llvm::makeArrayRef(GCCRegNames);
   }

 
     return X86TargetInfo::validateOperandSize(Constraint, Size);
   }
+  ArrayRef<Builtin::Info> getTargetBuiltins() const override {
+    return llvm::makeArrayRef(BuiltinInfoX86, clang::X86::LastX86CommonBuiltin -
+                                                  Builtin::FirstTSBuiltin + 1);
+  }
 };
 
 class NetBSDI386TargetInfo : public NetBSDTargetInfo<X86_32TargetInfo> {

     return X86TargetInfo::validateGlobalRegisterVariable(RegName, RegSize,
                                                          HasSizeMismatch);
   }
+  ArrayRef<Builtin::Info> getTargetBuiltins() const override {
+    return llvm::makeArrayRef(BuiltinInfoX86,
+                              X86::LastTSBuiltin - Builtin::FirstTSBuiltin);
+  }
 };
 
 // x86-64 Windows target

lib/CodeGen/CGBuiltin.cpp

     return getCmpIntrinsicCall(Intrinsic::x86_sse2_cmp_sd, 6);
   case X86::BI__builtin_ia32_cmpordsd:
     return getCmpIntrinsicCall(Intrinsic::x86_sse2_cmp_sd, 7);
+
+  case X86::BI__mulh:
+  case X86::BI__umulh: {
+    Value *LHS = EmitScalarExpr(E->getArg(0));
+    Value *RHS = EmitScalarExpr(E->getArg(1));
+    llvm::Type *ResType = ConvertType(E->getType());
+    llvm::Type *Int128Ty = llvm::IntegerType::get(getLLVMContext(), 128);
+
+    bool IsSigned = (BuiltinID == X86::BI__mulh);
+    LHS = Builder.CreateIntCast(LHS, Int128Ty, IsSigned);
+    RHS = Builder.CreateIntCast(RHS, Int128Ty, IsSigned);
+
+    Value *MulResult, *HigherBits;
+    if (IsSigned) {
+      MulResult = Builder.CreateNSWMul(LHS, RHS);
+      HigherBits = Builder.CreateAShr(MulResult, 64);
+    } else {
+      MulResult = Builder.CreateNUWMul(LHS, RHS);
+      HigherBits = Builder.CreateLShr(MulResult, 64);
+    }
+
+    HigherBits = Builder.CreateIntCast(HigherBits, ResType, IsSigned);
+    return HigherBits;
   }
+  }
 }
 
 

lib/Headers/intrin.h

 unsigned __int64 __lzcnt64(unsigned __int64);
 static __inline__
 void __movsq(unsigned long long *, unsigned long long const *, size_t);
+static __inline__
 __int64 __mulh(__int64, __int64);
 static __inline__
 unsigned __int64 __popcnt64(unsigned __int64);

   *_HighProduct = _FullProduct >> 64;
   return _FullProduct;
 }
-static __inline__ unsigned __int64 __DEFAULT_FN_ATTRS
-__umulh(unsigned __int64 _Multiplier, unsigned __int64 _Multiplicand) {
-  unsigned __int128 _FullProduct =
-      (unsigned __int128)_Multiplier * (unsigned __int128)_Multiplicand;
-  return _FullProduct >> 64;
-}
+static __inline__
+unsigned __int64 __umulh(unsigned __int64, unsigned __int64);
 
 #endif /* __x86_64__ */
 

test/CodeGen/ms-intrinsics.c

 #endif
 
 #if defined(__x86_64__)
+__int64 test__mulh(__int64 a, __int64 b) {
+  return __mulh(a, b);
+}
+// CHECK-X64-LABEL: define i64 @test__mulh(i64 %a, i64 %b)
+// CHECK-X64: = mul nsw i128 %
+
 unsigned __int64 test__umulh(unsigned __int64 a, unsigned __int64 b) {
   return __umulh(a, b);
 }
 // CHECK-X64-LABEL: define i64 @test__umulh(i64 %a, i64 %b)
 // CHECK-X64: = mul nuw i128 %
-
 #endif
 
 char test_InterlockedExchange8(char volatile *value, char mask) {

test/Sema/implicit-ms-builtin-decl.c

-// RUN: %clang_cc1 -fsyntax-only -verify %s -fms-extensions
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s -fms-extensions
+// RUN: %clang_cc1 -triple i386-unknown-unknown -fsyntax-only -verify %s -fms-extensions
 
 void f() {
-  (void)_byteswap_ushort(42); // expected-warning{{implicitly declaring library function '_byteswap_ushort}} \
+  (void)_byteswap_ushort(42); // expected-warning{{implicitly declaring library function '_byteswap_ushort'}} \
   // expected-note{{include the header <stdlib.h> or explicitly provide a declaration for '_byteswap_ushort'}}
-  (void)_byteswap_uint64(42LL); // expected-warning{{implicitly declaring library function '_byteswap_uint64}} \
+  (void)_byteswap_uint64(42LL); // expected-warning{{implicitly declaring library function '_byteswap_uint64'}} \
   // expected-note{{include the header <stdlib.h> or explicitly provide a declaration for '_byteswap_uint64'}}
 }
 

   (void)_byteswap_ushort(42);
   (void)_byteswap_uint64(42LL);
 }
+
+#if defined(__x86_64__)
+void h() {
+  (void)__mulh(21, 2);  // expected-warning{{implicitly declaring library function '__mulh'}} \
+  // expected-note{{include the header <intrin.h> or explicitly provide a declaration for '__mulh'}}
+  (void)__umulh(21, 2); // expected-warning{{implicitly declaring library function '__umulh'}} \
+  // expected-note{{include the header <intrin.h> or explicitly provide a declaration for '__umulh'}}
+}
+
+long long __mulh(long long, long long);
+unsigned long long __umulh(unsigned long long, unsigned long long);
+
+void i() {
+  (void)__mulh(21, 2);
+  (void)__umulh(21, 2);
+}
+#endif
+
+#if defined(i386)
+void h() {
+  (void)__mulh(21LL, 2LL);  // expected-warning{{implicit declaration of function '__mulh' is invalid}}
+  (void)__umulh(21ULL, 2ULL);  // expected-warning{{implicit declaration of function '__umulh' is invalid}}
+}
+#endif