Add Clang support for remaining integer divide and permute instructions from ISA 2.06

[nemanjai]

Summary

This patch provides some of the missing instructions from ISA 2.06. Namely the integer divide extended instructions (and associated builtins) are provided. The record-form variants are provided but can be accessed through inline assembly only.
Since these instructions are introduced with ISA 2.06, it is important that they not be emitted for targets that implement an older ISA. Also, since they are exposed in the front end through builtinins, it is important that the front end knows not to emit the intrinsics to the back end if the target does not support them. Therefore, this patch provides the ability to test whether the target is Power 7 and up (and Power 8 and up) and use of the builtins is diagnosed on older CPUs.
This diverges from GCC behaviour of controlling the builtins and instructions with -mpopcntd since that option has very little to do with these instructions/builtins.

Note: in a general case, a newer ISA is a superset of the previous ISA. However, instructions are sometimes removed from the ISA. This raises the question whether emitting instructions/builtins based on "IsPwr7Up", et. al. is the right approach. In my opinion, it is unlikely that something that is added in say ISA 2.06 will be removed any time soon, but I welcome other opinions.

[wschmidt]

Hi Nemanja,

Just a few comments inline.

lib/Basic/Targets.cpp

1267

These strings should not be capitalized, to maintain consistency. As in the other review, I'm not excited about the name.

I really question whether these should be features. Doesn't that imply that these become things that can be controlled independently as -mattr=+/- for llc? I don't think we want that.

1271

mmm, even if we keep this approach, I'm not sure about the Is64Bit one. Surely these aren't the only built-ins that only work for 64-bit. If you're going to add this it needs to be complete and consistent, which is far more than what you're going for here. My inclination is to leave it out and consider whether a full patch for this is sensible down the road.

lib/CodeGen/CGBuiltin.cpp

6390 ↗

(On Diff #22123)

I'd prefer that the conditions you've encoded as features just be tested here, rather than using the feature mechanism, but this is a call for people with more experience with this than me.

[nemanjai]

lib/Basic/Targets.cpp

1267

Good point about the capitalization. I just used the same names as the bool variables. I can change this.

Regarding the second point, I was careful not to expose these features outside of the target info. They will not become features that have a corresponding Clang option nor will they give the user any control over turning these on/off. This just provides a string-based query for the data members that I added. Essentially, this is a unified "getter" that provides a name-based query.
All that being said, I do see your point. Everything else in this string->bool map is an actual feature that a user has control over. These new ones deviate from that. I am open to suggestions on how to achieve what we are trying to achieve here (namely, control over exposing builtins for non-optional instructions).

1271

I will investigate whether other 64-bit only builtins provide any diagnostics if used in code that is compiled at 32-bit. Thanks for bringing this up.

lib/CodeGen/CGBuiltin.cpp

6390 ↗

(On Diff #22123)

I am certainly not opposed to this. Doing so would negate the need for the "fake features" from above. However, the reason for doing it this way was in anticipation of any future CPU's that are a superset of Power7...
Say we implement 5 builtins that are only available on Power7. Then another 17 that are available on Power8. We then get CPU X which is a superset of both. We would need to track down all the places where we check for the correct CPU. Using this approach puts this check in one place.
However, all this is just to explain why I did it this way. If you (or other interested parties) feel that I shouldn't, I'd be happy to change it.

[nemanjai]

lib/Basic/Targets.cpp

1271

OK, as it turns out, this seems to largely be ignored by other targets (and we do not seem to have any builtins implemented that are available on 64-bit only.
I personally feel that diagnosing this is better than just letting the back end crash.
Example from Arm:

$ cat t.c 
void test_clrex() {
  __builtin_arm_clrex();
}
$ clang t.c --target=arm-apple-ios7.0 -c
fatal error: error in backend: Cannot select: intrinsic %llvm.arm.clrex
clang: error: clang frontend command failed with exit code 70 (use -v to see invocation)
clang version 3.7.0 (trunk 231831) (llvm/trunk 231828:231843M)
Target: arm-apple-ios7.0
Thread model: posix
clang: note: diagnostic msg: PLEASE submit a bug report to http://llvm.org/bugs/ and include the crash backtrace, preprocessed source, and associated run script.
clang: note: diagnostic msg: 
********************

PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
Preprocessed source(s) and associated run script(s) are located at:
clang: note: diagnostic msg: /tmp/t-4a719a.c
clang: note: diagnostic msg: /tmp/t-4a719a.sh
clang: note: diagnostic msg: 

********************

Similar example with this changeset:

$ cat a.c 
long long test_divde(void) {
  return __builtin_divde(74LL, 32LL);
}
$ clang a.c -c --target=powerpc-unknown-unknown -mcpu=pwr7
a.c:2:10: error: This builtin is only available on 64-bit targets
  return __builtin_divde(74LL, 32LL);
         ^
1 error generated.

I personally feel that a diagnostic identifying the problem is more helpful to the user than a message that the back end crashed and suggestion to open a bug report.

[hfinkel]

lib/Basic/Targets.cpp

745

We don't need the 'Up' in the names here; I think we can generally assume that newer cores will provide backward compatibility, and only when they don't would we need some kind of HasP8Only feature.

747

Please remove this. We can get at the current pointer size from generic code.

1271

I agree, we should generate an error in the frontend instead of just crashing in the backend.

lib/CodeGen/CGBuiltin.cpp

6390 ↗

(On Diff #22123)

I have a weak preference for keeping the cpu -> feature logic localized to the target code, so I think this general scheme is good.

[hfinkel]

lib/Basic/Targets.cpp

1267

Also, please keep the existing naming convention for these (lowercase, etc.), and we don't need all of the aliases. How about just adding features for 'isa-2.06', 'isa-2.07', etc.?

[nemanjai]

Updated the patch to address the comments and to synchronize the way the builtins are enabled between the front end and back end.

[nemanjai]

lib/Basic/Targets.cpp

746

Rather than having the ISA level here, I've opted for the same names in the front end and the back end. If we choose to add group features to the back end for the ISA level, we can revisit doing it here as well.

[wschmidt]

LGTM with one small change.

lib/CodeGen/CGBuiltin.cpp

6411 ↗

(On Diff #22691)

You need to split these conditions so that the bpermd feature only controls BI_ _builtin_bpermd and the extdiv feature only controls BI_ _builtin_div*.

[nemanjai]

lib/CodeGen/CGBuiltin.cpp

6411 ↗

(On Diff #22691)

Good point! We don't want to suppress the diagnostics for the other when only one is enabled :-). Not sure what I was thinking here. I'll fix it in the committed patch. Or would you like another review for just this (of course, if there are other comments this question is moot)?

[hfinkel]

lib/CodeGen/CGBuiltin.cpp

6411 ↗

(On Diff #22691)

Please note Richards comments on the HTM commit; these error checks should likely be moved into Sema. So, yes, another review round is probably necessary.

[nemanjai]

lib/CodeGen/CGBuiltin.cpp

6411 ↗

(On Diff #22691)

OK, I see what is needed. Kit is putting in the stuff for HTM. Rather than us clobbering each other, I'll wait until he has that done and update this review then. I'll also use the same framework for the crypto ones at that time. Thanks Hal.

[nemanjai]

Moved the diagnostics to Sema.

[wschmidt]

LGTM.

[nemanjai]

Committed revision 234547.

include/clang/Basic/BuiltinsPPC.def

 
 BUILTIN(__builtin_set_texasr, "vLUi", "c")
 BUILTIN(__builtin_set_texasru, "vLUi", "c")
 BUILTIN(__builtin_set_tfhar, "vLUi", "c")
 BUILTIN(__builtin_set_tfiar, "vLUi", "c")
 
 BUILTIN(__builtin_ttest, "LUi", "")
 
+// Scalar built-ins
+BUILTIN(__builtin_divwe, "SiSiSi", "")
+BUILTIN(__builtin_divweu, "UiUiUi", "")
+BUILTIN(__builtin_divde, "SLLiSLLiSLLi", "")
+BUILTIN(__builtin_divdeu, "ULLiULLiULLi", "")
+BUILTIN(__builtin_bpermd, "SLLiSLLiSLLi", "")
+
 // FIXME: Obviously incomplete.
 
 #undef BUILTIN

include/clang/Basic/DiagnosticSemaKinds.td

def note_neon_vector_initializer_non_portable : Note<
   "consider using vld1_%0%1() to initialize a vector from memory, or "
   "vcreate_%0%1() to initialize from an integer constant">;
 def note_neon_vector_initializer_non_portable_q : Note<
   "consider using vld1q_%0%1() to initialize a vector from memory, or "
   "vcombine_%0%1(vcreate_%0%1(), vcreate_%0%1()) to initialize from integer "
   "constants">;
 def err_systemz_invalid_tabort_code : Error<
   "invalid transaction abort code">;
+def err_64_bit_builtin_32_bit_tgt : Error<
+  "this builtin is only available on 64-bit targets">;
+def err_ppc_builtin_only_on_pwr7 : Error<
+  "this builtin is only valid on POWER7 or later CPUs">;
 
 def err_builtin_longjmp_unsupported : Error<
   "__builtin_longjmp is not supported for the current target">;
 def err_builtin_setjmp_unsupported : Error<
   "__builtin_setjmp is not supported for the current target">;
 
 def err_builtin_longjmp_invalid_val : Error<
   "argument to __builtin_longjmp must be a constant 1">;

lib/Basic/Targets.cpp

class PPCTargetInfo : public TargetInfo {
   static const char * const GCCRegNames[];
   static const TargetInfo::GCCRegAlias GCCRegAliases[];
   std::string CPU;
 
   // Target cpu features.
   bool HasVSX;
   bool HasP8Vector;
   bool HasP8Crypto;
   bool HasQPX;
   bool HasHTM;
+  bool HasBPERMD;
+  bool HasExtDiv;
 
 protected:
   std::string ABI;
 
 public:
   PPCTargetInfo(const llvm::Triple &Triple)
     : TargetInfo(Triple), HasVSX(false), HasP8Vector(false),
-      HasP8Crypto(false), HasQPX(false), HasHTM(false) {
+      HasP8Crypto(false), HasQPX(false), HasHTM(false),
+      HasBPERMD(false), HasExtDiv(false) {
     BigEndian = (Triple.getArch() != llvm::Triple::ppc64le);
     LongDoubleWidth = LongDoubleAlign = 128;
     LongDoubleFormat = &llvm::APFloat::PPCDoubleDouble;
   }
 
   /// \brief Flags for architecture specific defines.
   typedef enum {
     ArchDefineNone  = 0,
for (unsigned i = 0, e = Features.size(); i !=e; ++i) {
 
     StringRef Feature = StringRef(Features[i]).substr(1);
 
     if (Feature == "vsx") {
       HasVSX = true;
       continue;
     }
 
+    if (Feature == "bpermd") {
+      HasBPERMD = true;
+      continue;
+    }
+
+    if (Feature == "extdiv") {
+      HasExtDiv = true;
+      continue;
+    }
+
     if (Feature == "power8-vector") {
       HasP8Vector = true;
       continue;
     }
 
     if (Feature == "crypto") {
       HasP8Crypto = true;
       continue;
void PPCTargetInfo::getDefaultFeatures(llvm::StringMap<bool> &Features) const {
   Features["crypto"] = llvm::StringSwitch<bool>(CPU)
     .Case("ppc64le", true)
     .Case("pwr8", true)
     .Default(false);
   Features["power8-vector"] = llvm::StringSwitch<bool>(CPU)
     .Case("ppc64le", true)
     .Case("pwr8", true)
     .Default(false);
+  Features["bpermd"] = llvm::StringSwitch<bool>(CPU)
+    .Case("ppc64le", true)
+    .Case("pwr8", true)
+    .Case("pwr7", true)
+    .Default(false);
+  Features["extdiv"] = llvm::StringSwitch<bool>(CPU)
+    .Case("ppc64le", true)
+    .Case("pwr8", true)
+    .Case("pwr7", true)
+    .Default(false);
 }
 
 bool PPCTargetInfo::hasFeature(StringRef Feature) const {
   return llvm::StringSwitch<bool>(Feature)
     .Case("powerpc", true)
     .Case("vsx", HasVSX)
     .Case("power8-vector", HasP8Vector)
     .Case("crypto", HasP8Crypto)
     .Case("qpx", HasQPX)
     .Case("htm", HasHTM)
+    .Case("bpermd", HasBPERMD)
+    .Case("extdiv", HasExtDiv)
     .Default(false);
 }
 
 const char * const PPCTargetInfo::GCCRegNames[] = {
   "r0", "r1", "r2", "r3", "r4", "r5", "r6", "r7",
   "r8", "r9", "r10", "r11", "r12", "r13", "r14", "r15",
   "r16", "r17", "r18", "r19", "r20", "r21", "r22", "r23",
   "r24", "r25", "r26", "r27", "r28", "r29", "r30", "r31",
   "f0", "f1", "f2", "f3", "f4", "f5", "f6", "f7",

lib/Sema/SemaChecking.cpp

bool Sema::CheckMipsBuiltinFunctionCall(unsigned BuiltinID, CallExpr *TheCall) {
   case Mips::BI__builtin_mips_prepend: i = 2; l = 0; u = 31; break;
   }
 
   return SemaBuiltinConstantArgRange(TheCall, i, l, u);
 }
 
 bool Sema::CheckPPCBuiltinFunctionCall(unsigned BuiltinID, CallExpr *TheCall) {
   unsigned i = 0, l = 0, u = 0;
+  bool Is64BitBltin = BuiltinID == PPC::BI__builtin_divde ||
+                      BuiltinID == PPC::BI__builtin_divdeu ||
+                      BuiltinID == PPC::BI__builtin_bpermd;
+  bool IsTarget64Bit = Context.getTargetInfo()
+                              .getTypeWidth(Context
+                                            .getTargetInfo()
+                                            .getIntPtrType()) == 64;
+  bool IsBltinExtDiv = BuiltinID == PPC::BI__builtin_divwe ||
+                       BuiltinID == PPC::BI__builtin_divweu ||
+                       BuiltinID == PPC::BI__builtin_divde ||
+                       BuiltinID == PPC::BI__builtin_divdeu;
+
+  if (Is64BitBltin && !IsTarget64Bit)
+      return Diag(TheCall->getLocStart(), diag::err_64_bit_builtin_32_bit_tgt)
+             << TheCall->getSourceRange();
+
+  if ((IsBltinExtDiv && !Context.getTargetInfo().hasFeature("extdiv")) ||
+      (BuiltinID == PPC::BI__builtin_bpermd &&
+       !Context.getTargetInfo().hasFeature("bpermd")))
+    return Diag(TheCall->getLocStart(), diag::err_ppc_builtin_only_on_pwr7)
+           << TheCall->getSourceRange();
+
   switch (BuiltinID) {
   default: return false;
   case PPC::BI__builtin_altivec_crypto_vshasigmaw:
   case PPC::BI__builtin_altivec_crypto_vshasigmad:
     return SemaBuiltinConstantArgRange(TheCall, 1, 0, 1) ||
            SemaBuiltinConstantArgRange(TheCall, 2, 0, 15);
   case PPC::BI__builtin_tbegin:
   case PPC::BI__builtin_tend: i = 0; l = 0; u = 1; break;

test/CodeGen/builtins-ppc-p7-disabled.c

+// REQUIRES: powerpc-registered-target
+// RUN: not %clang_cc1 -triple powerpc64-unknown-unknown \
+// RUN: -target-cpu pwr6 -emit-llvm %s -o - 2>&1 \
+// RUN: | FileCheck %s
+
+// RUN: not %clang_cc1 -triple powerpc-unknown-unknown -emit-llvm %s -o - 2>&1 \
+// RUN: -target-cpu pwr7 | FileCheck %s -check-prefix=CHECK-32
+
+void call_p7_builtins(void)
+{
+  int a = __builtin_divwe(33, 11);
+  unsigned int b = __builtin_divweu(33U, 11U);
+  unsigned long long d = __builtin_divde(33ULL, 11ULL);
+  unsigned long long e = __builtin_divdeu(33ULL, 11ULL);
+  unsigned long long f = __builtin_bpermd(33ULL, 11ULL);
+}
+
+// CHECK: error: this builtin is only valid on POWER7 or later CPUs
+// CHECK: __builtin_divwe
+// CHECK: error: this builtin is only valid on POWER7 or later CPUs
+// CHECK: __builtin_divweu
+// CHECK: error: this builtin is only valid on POWER7 or later CPUs
+// CHECK: __builtin_divde
+// CHECK: error: this builtin is only valid on POWER7 or later CPUs
+// CHECK: __builtin_divdeu
+// CHECK: error: this builtin is only valid on POWER7 or later CPUs
+// CHECK: __builtin_bpermd
+// CHECK-32: error: this builtin is only available on 64-bit targets
+// CHECK-32: __builtin_divde
+// CHECK-32: error: this builtin is only available on 64-bit targets
+// CHECK-32: __builtin_divdeu
+// CHECK-32: error: this builtin is only available on 64-bit targets
+// CHECK-32: __builtin_bpermd

test/CodeGen/builtins-ppc-p7.c

+// REQUIRES: powerpc-registered-target
+// RUN: %clang_cc1 -triple powerpc64-unknown-unknown -target-cpu pwr7 \
+// RUN: -emit-llvm %s -o - | FileCheck %s
+
+// RUN: %clang_cc1 -triple powerpc64le-unknown-unknown -target-cpu pwr8 \
+// RUN: -emit-llvm %s -o - | FileCheck %s
+
+// CHECK-LABEL: define signext i32 @test_divwe
+int test_divwe(void)
+{
+  int a = 74;
+  int b = 32;
+  return __builtin_divwe(a, b);
+// CHECK @llvm.ppc.divwe
+}
+
+// CHECK-LABEL: define zeroext i32 @test_divweu
+unsigned int test_divweu(void)
+{
+  unsigned int a = 74;
+  unsigned int b = 32;
+  return __builtin_divweu(a, b);
+// CHECK @llvm.ppc.divweu
+}
+
+// CHECK-LABEL: define i64 @test_divde
+long long test_divde(void)
+{
+  long long a = 74LL;
+  long long b = 32LL;
+  return __builtin_divde(a, b);
+// CHECK @llvm.ppc.divde
+}
+
+// CHECK-LABEL: define i64 @test_divdeu
+unsigned long long test_divdeu(void)
+{
+  unsigned long long a = 74ULL;
+  unsigned long long b = 32ULL;
+  return __builtin_divdeu(a, b);
+// CHECK @llvm.ppc.divdeu
+}
+
+// CHECK-LABEL: define i64 @test_bpermd
+long long test_bpermd(void)
+{
+  long long a = 74LL;
+  long long b = 32LL;
+  return __builtin_bpermd(a, b);
+// CHECK @llvm.ppc.bpermd
+}
+