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

Bill Schmidt wschmidt at linux.vnet.ibm.com
Wed Mar 18 14:00:28 PDT 2015


Hi Nemanja,

Just a few comments inline.


REPOSITORY
  rL LLVM

================
Comment at: lib/Basic/Targets.cpp:1213
@@ -1203,1 +1212,3 @@
     .Case("crypto", HasP8Crypto)
+    .Case("IsPwr7Up", IsPwr7Up)
+    .Case("IsISA206Up", IsPwr7Up)
----------------
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.

================
Comment at: lib/Basic/Targets.cpp:1217
@@ -1204,1 +1216,3 @@
+    .Case("IsISA207Up", IsPwr8Up)
+    .Case("Is64Bit", Is64Bit)
     .Default(false);
----------------
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.

================
Comment at: lib/CodeGen/CGBuiltin.cpp:6390
@@ +6389,3 @@
+                        BuiltinID == PPC::BI__builtin_bpermd;
+    if(!getTarget().hasFeature("IsPwr7Up")) {
+      CGM.Error(E->getExprLoc(),
----------------
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.

http://reviews.llvm.org/D8398

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the cfe-commits mailing list