This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by nemanjai on Mar 17 2015, 8:05 PM.

Details

Summary

This is the back end portion of Review D8398. Similarly to the front end, the back end got flags for checking if the target implements ISA 2.06 and up (and whether it is a Power7 and up CPU), etc.

The integer divide instructions that were already implemented were changed slightly since both the record form and the non-record form were specified as "First" and "Cracked". However, Book IV states that the non-record forms are not cracked. So I have changed the multiclass to specify this.
Finally, the record form instructions are implemented only for [inline] assembly and no builtins were provided. This is consistent with GCC. As noted in the front end review, the -mpopcntd option has no effect on these (in contrast with GCC).

Diff Detail

Repository
rL LLVM

Event Timeline

nemanjai updated this revision to Diff 22152.Mar 17 2015, 8:05 PM
nemanjai retitled this revision from to Add LLVM support for remaining integer divide and permute instructions from ISA 2.06.
nemanjai updated this object.
nemanjai edited the test plan for this revision. (Show Details)
nemanjai set the repository for this revision to rL LLVM.
nemanjai added a subscriber: Unknown Object (MLST).
wschmidt edited edge metadata.Mar 18 2015, 1:39 PM

Hi Nemanja,

Just a few minor inline comments. Good-looking patch otherwise!

include/llvm/IR/IntrinsicsPowerPC.td
50

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.

lib/Target/PowerPC/PPCInstr64Bit.td
630

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...

lib/Target/PowerPC/PPCInstrInfo.td
717

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.

lib/Target/PowerPC/README.txt
626

Nit: No hyphen in "haphazard."

hfinkel added inline comments.Mar 18 2015, 1:46 PM
lib/Target/PowerPC/PPCInstrInfo.td
717

So true ;) -- Yea, I don't like it either.

Just make a target feature, and name it after some characteristic instruction. HasDIVDE perhaps?

lib/Target/PowerPC/PPCSubtarget.cpp
148

No... target features in PPC.td please.

nemanjai added inline comments.Mar 18 2015, 6:43 PM
include/llvm/IR/IntrinsicsPowerPC.td
50

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).

lib/Target/PowerPC/PPCInstr64Bit.td
630

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.

lib/Target/PowerPC/PPCInstrInfo.td
717

I thought it was funny when I came up with the name :-). 7Up, but the Power version. I will change it.

lib/Target/PowerPC/PPCSubtarget.cpp
148

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.

lib/Target/PowerPC/README.txt
626

Oops, sorry, I'll fix it.

hfinkel added inline comments.Mar 18 2015, 7:57 PM
lib/Target/PowerPC/PPCSubtarget.cpp
148

Don't worry about it. Other targets are also running up against this limit, and there is a patchset under review to increase it.

nemanjai updated this revision to Diff 22351.Mar 20 2015, 8:44 AM
nemanjai edited edge metadata.

Addressed comments from the previous review. Please note the resolution I've implemented for the comments about features that control these instructions. I felt that picking a characteristic instruction is also somewhat misleading (this patch is mostly divide instructions, but the next one will have more completely unrelated ones). So what I did instead is implement a feature that will enable all the features of a full ISA 2.06 implementation with -mattr=+isa-206 (subject to change if you would prefer a different string). The plan then is to do the same for ISA 2.07 in a later patch and if everyone agrees with this one.

Hi Nemanja,

Sorry to be a pain, but I don't support FeatureISA206 / FeatureISA207. See inline comment. :/ Hal, your thoughts?

Bill

lib/Target/PowerPC/PPC.td
163 ↗(On Diff #22351)

I don't care for the FeatureISA206 choice. It contradicts the existing design (there are plenty of features called out that are also in ISA 2.06, so why is this one special?), and is redundant with Power7FeatureList (if you have a Power7FeatureList, shouldn't it just be previous features + FeatureISA206 under this philosophy?).

I think you should go back to Hal's suggestion and provide relevant features that are grouped in a reasonable way. For this patch, that would be something like FeatureExtDiv and FeatureBitPerm. I don't think we have a need to disrupt the existing design.

hfinkel added inline comments.Mar 23 2015, 1:37 PM
lib/Target/PowerPC/PPC.td
163 ↗(On Diff #22351)

I agree with Bill agreeing with me ;)

That having been said, I would like to get a better handle on the features associated with ISA revisions, and group those together. So please add fine-grained features (for one thing, we'll likely have CodeGen support for these at some point, at least bpermd). As a separate change, we can add feature groups for the ISA revisions, and then use those to define the default sets of features for the processors defined.

nemanjai added inline comments.Mar 23 2015, 2:04 PM
lib/Target/PowerPC/PPC.td
163 ↗(On Diff #22351)

OK, so to summarize:

  • In this patch, add fine-grained control for the instructions that are implemented (I will add FeatureExtDiv and FeatureBitPerm - should I add a "d" at the end of that?)
  • In a future patch, provide something along the lines of these feature groups (FeatureISA206 in this case)
wschmidt added inline comments.Mar 23 2015, 2:14 PM
lib/Target/PowerPC/PPC.td
163 ↗(On Diff #22351)

You can add a "D" if you like; I don't care either way. It protects against a future feature that does bit permute on another data type, I suppose.

hfinkel added inline comments.Mar 23 2015, 2:21 PM
lib/Target/PowerPC/PPC.td
163 ↗(On Diff #22351)

Yes, that sounds like a good plan. I think adding the D makes sense, I'd just name it, like others, based on the particular instruction: FeatureBPERMD since it refers to the particular instruction.

nemanjai updated this revision to Diff 22693.Mar 25 2015, 7:02 PM

I've reverted back to using fine grained target features as requested in the last review. Since I am adding more Power7 features, I've condensed the feature list for pwr7 in a similar fashion to the way the feature list for pwr8 was done.

wschmidt accepted this revision.Mar 26 2015, 6:25 AM
wschmidt edited edge metadata.

LGTM. Thanks for the revisions!

This revision is now accepted and ready to land.Mar 26 2015, 6:25 AM
nemanjai closed this revision.Apr 9 2015, 4:58 PM

Committed revision 234546.