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

Nemanja Ivanovic nemanja.i.ibm at gmail.com
Wed Mar 18 18:43:23 PDT 2015


REPOSITORY
  rL LLVM

================
Comment at: include/llvm/IR/IntrinsicsPowerPC.td:50
@@ +49,3 @@
+                                 [IntrNoMem]>;
+  def int_ppc_bpermd : Intrinsic<[llvm_i64_ty], [llvm_i64_ty, llvm_i64_ty],
+                                 [IntrNoMem]>;
----------------
wschmidt wrote:
> OK, this is a real nit, but -- perhaps add a blank line before bpermd since this one is not a divide-extend as the comment would seem to imply.
This makes sense. I should probably have picked up on this. I'll add a blank line (or consider whether this definition belongs in a different part of the file).

================
Comment at: lib/Target/PowerPC/PPCInstr64Bit.td:630
@@ +629,3 @@
+                      "divde. $rT, $rA, $rB", IIC_IntDivD,
+                      []>, isDOT, PPC970_DGroup_Cracked, isPPC64,
+                      Requires<[IsPwr7Up]>;
----------------
wschmidt wrote:
> As discussed offline, I am bothered that these are not marked first-in-group but the regular divides are.  Please check with Pat Haugen on correctness.
> 
> Long term, it makes no sense to have a single set of flags like this for each instruction, since these characteristics can and do change from one implementation to the next.  But that will be a different patch...
Thanks for bringing this up Bill. As it turns out, ALL cracked instructions are first in the dispatch group. For now, I will add PPC970_DGroup_First here as well. In the future, we can revisit how to accomplish this in a way that is specific to the CPU.

================
Comment at: lib/Target/PowerPC/PPCInstrInfo.td:717
@@ -716,1 +716,3 @@
 def NaNsFPMath   : Predicate<"!TM.Options.NoNaNsFPMath">;
+def IsPwr7Up : Predicate<"PPCSubTarget->isPwr7Up()">;
+def IsPwr8Up : Predicate<"PPCSubTarget->isPwr8Up()">;
----------------
hfinkel wrote:
> wschmidt wrote:
> > I'm a tiny bit not enthused about the name -- IsPwr7OrHigher, perhaps?  Pwr7Up sounds like an energy drink. ;)  If nobody else dislikes it, leave it as is, though.
> So true ;) -- Yea, I don't like it either.
> 
> Just make a target feature, and name it after some characteristic instruction. HasDIVDE perhaps?
> 
I thought it was funny when I came up with the name :-). 7Up, but the Power version. I will change it.

================
Comment at: lib/Target/PowerPC/PPCSubtarget.cpp:148
@@ +147,3 @@
+  IsPwr8Up = CPUName == "ppc64le" || CPUName == "pwr8";
+  IsPwr7Up = CPUName == "pwr7" || IsPwr8Up;
+  IsISA206Up = IsPwr7Up;
----------------
hfinkel wrote:
> No... target features in PPC.td please.
> 
In response to this comment and the one about the predicate...

The brief history of this design decision is that target features have a limitation. Namely, the target is limited to 64 of them. Now I haven't come across any fundamental reason for this, it is just that tblgen does not handle any more. The reason it doesn't is that in the generated code, each feature is a uint64_t entity that is 1 left-shifted by n (where n is the feature's zero-based ordinal number). At my last check, we were at around 58 features. So I did this to avoid blowing past the 64 feature limit for things that can easily be encoded in the C++ portion.
If everyone agrees that target features are the way to go and we don't want to worry about defining too many of these features, I'll be happy to change it.

================
Comment at: lib/Target/PowerPC/README.txt:626
@@ +625,3 @@
+//===-------------------------------------------------------------------------===
+Naming convention for instruction formats is very hap-hazard.
+We have agreed on a naming scheme as follows:
----------------
wschmidt wrote:
> Nit:  No hyphen in "haphazard."
Oops, sorry, I'll fix it.

http://reviews.llvm.org/D8406

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






More information about the llvm-commits mailing list