Page MenuHomePhabricator

Power9 - Implement byte comparison and count trailing zero instructions
ClosedPublic

Authored by nemanjai on Mar 3 2016, 7:09 AM.

Details

Summary

This patch implements the following instructions:
cmprb
cmpeqb
cnttzw
cnttzw.
cnttzd
cnttzd.

It also implements a list of P9 specific target features that we will add to when implementing new features so that we don't forget any.
Finally, the patterns for the count trailing zeros are provided because they're obvious.

Diff Detail

Repository
rL LLVM

Event Timeline

nemanjai updated this revision to Diff 49734.Mar 3 2016, 7:09 AM
nemanjai retitled this revision from to Power9 - Implement byte comparison and count trailing zero instructions.
nemanjai updated this object.
nemanjai added reviewers: kbarton, hfinkel, cycheng, amehsan.
nemanjai set the repository for this revision to rL LLVM.
nemanjai added a subscriber: llvm-commits.
nemanjai updated this revision to Diff 49737.Mar 3 2016, 7:17 AM

I forgot to add comments about this to README_P9.txt.

amehsan edited edge metadata.Mar 4 2016, 6:22 AM

just a few minor nits. I did not review the tests though. I am going to learn that part of the code today :)

lib/Target/PowerPC/PPC.td
154–159

It will make the code more readable to have something in the name or description of the feature that indicates the feature was introduced in P9. I know we do not have it for many features, but as features are added, some extra clarity will be helpful.

lib/Target/PowerPC/PPCInstr64Bit.td
570

any reason for different indentation of 'Requires' clause? I noticed that sometimes we use this different indentation for 'Requires' and sometimes not. Couldn't figure out justification for having inconsistent indentation.

592

same question here.

626

I think you introduced an extra space unintentionally.

lib/Target/PowerPC/PPCInstrFormats.td
767

You can probably remove the comment. Does it help the reader to know this class is very similar to XForm_17?

770

different indentation in here and the other class that you defined right before this.

cycheng added inline comments.Mar 4 2016, 7:28 AM
lib/Target/PowerPC/PPCInstr64Bit.td
567

These instructions are enclosed in "let PPC970_Unit = 1 in { ... }"

It looks like PPC970 indicating specific processor, i.e. G5, and it's purpose seems for PPCHazardRecognizer970 instruction scheduling implementation, so I don't know whether "let PPC970_Unit ..." is required for Power9 instructions or not.

567

Should we move CMPRB to PPCInstrInfo.td, and provide CMPRB8 definition here? Because it looks like 32-bit instructions:

(RA)56:63
(RB)32:63
lib/Target/PowerPC/PPCInstrFormats.td
767

I think it's helpful : D

nemanjai marked an inline comment as done.Mar 5 2016, 11:30 PM
nemanjai added inline comments.
lib/Target/PowerPC/PPC.td
154–159

I think we generally did not mention any of the processors a feature is implemented on because such features can become part of multiple chips. For example, FeatureLDBRX is available on Power7, but it's also available on the A2 chip.

That being said, I'm not terribly opposed to having P9 in the name, but I think I'll defer the final decision to @hfinkel.

lib/Target/PowerPC/PPCInstr64Bit.td
567

This is a good point. I initially implemented it that way and then removed it from the 32-bit definition. However, both the Hi and Lo range delimiters are in the lower 32-bits of the registers so this should be perfectly usable in 32-bit mode.
I'll add it there.

567

I have added them to this section assuming it is sufficiently similar to these instructions from the standpoint of scheduling. However, I don't understand the details of how this information is used. Hopefully @hfinkel can provide some guidance here.

570

I realize that there are some places where the "Requires" base class appears on the start of a line and it is aligned with the name of the instruction. However, since the definition of the instruction inherits from two classes (namely, X_BF3_L1_RS5_RS5 and Requires in this case), I feel that both classes inherited should be aligned after the colon.

626

The intention was to align everything to the right of "58" to the newly added instruction (similarly to CNTLZW in the 32-bit instruction definition file). I know it's a rather unrelated change, I just thought it looked nicer all ligned up :).

lib/Target/PowerPC/PPCInstrFormats.td
767

Me too. I think it's a quick reference for someone that is looking for a form to use and XForm_17 is almost right. Also, if we ever get around to renaming forms to have a common scheme, it can be helpful.

770

That's a good point. I don't know how this one got out. I'll fix it. Thanks.

kbarton added inline comments.Mar 7 2016, 8:45 PM
lib/Target/PowerPC/PPC.td
160

I don't see the need for adding a subtarget feature for these instructions. Why is this necessary?

nemanjai added inline comments.Mar 7 2016, 9:47 PM
lib/Target/PowerPC/PPC.td
160

It is not necessary to add individual features for these instructions. However, it doesn't seem right to guard these by the only existing Power9 features (namely Power9Vector, Power9Altivec). Also, adding a single feature for all Power9 scalar instructions seems to break with tradition.
Could you please suggest an alternative to defining these two features?

kbarton accepted this revision.Mar 29 2016, 1:25 PM
kbarton edited edge metadata.

Please replace the FeatureCharCmp and FeatureCTTZ features with the more generic ISA3.0 feature (or something similar).
With this change, this LGTM.

lib/Target/PowerPC/PPC.td
160

Based on discussions today we agreed to add a generic feature to indicate ISA3.0. The name should reflect the actual ISA version number, and not be tied to any specific chip implementation (i.e., P9). The feature definitions for P9 (e.g., P9-vector and P9-altivec) should prereq this ISA3.0 feature. Similarly, the definition of the P9 sub target should include this as well.

This revision is now accepted and ready to land.Mar 29 2016, 1:25 PM
nemanjai closed this revision.Apr 13 2016, 12:01 PM

Committed revision 266228.