This is an archive of the discontinued LLVM Phabricator instance.

[Power9] Implement add-pc, multiply-add, modulo, extend-sign-shift, random number, set bool, and dfp test significance
ClosedPublic

Authored by cycheng on Mar 4 2016, 5:44 AM.

Details

Summary

This patch implement the following instructions:

  • addpcis subpcis
  • maddhd maddhdu maddld
  • modsw moduw modsd modud
  • darn
  • extswsli extswsli.
  • setb
  • dtstsfi dtstsfiq

Total 15 instructions

Diff Detail

Event Timeline

cycheng updated this revision to Diff 49821.Mar 4 2016, 5:44 AM
cycheng retitled this revision from to [Power9] Implement add-pc, multiply-add, modulo, extend-sign-shift, random number, set bool, and dfp test significance.
cycheng updated this object.
cycheng added a subscriber: llvm-commits.
nemanjai added inline comments.Mar 7 2016, 12:33 PM
lib/Target/PowerPC/PPCInstr64Bit.td
1262

Please move this definition up to PPCInstrFormats.td since it is used not only in this file, but in PPCInstInfo.td. I realize that everything defined here is available in PPCInstrInfo.td after the include line but nonetheless, I feel that common definitions should go in the common file.

lib/Target/PowerPC/PPCInstrInfo.td
4151

I'm just curious why the decision to provide one extended mnemonic (subpcis) but not the other (lnia).

4161

How does this encode the floating register pair? Actually, I just noticed that you have a comment regarding this in the README. SystemZ has already done something like this (the FP128 and GR128 register classes). Perhaps we should do something along those lines.

This will also help us implement the instructions that are missing from previous versions of the ISA (lq, lfdp, lqarx, et. al.).

cycheng marked 2 inline comments as done.Mar 9 2016, 5:10 AM
cycheng added inline comments.
lib/Target/PowerPC/PPCInstrInfo.td
4151

I missed it, thanks : P
That means gcc also forgot to implement it @@

4161

Thanks your guide, I found useful information from SystemZ and Sparc.

I did something like this, passed building, but not as my expectation:

// Paired F8RC
def subreg_l64   : SubRegIndex<64, 0>;
def subreg_h64   : SubRegIndex<64, 64>;

class DQPR<FPR even, FPR odd, string n> : PPCReg<n> {
  let HWEncoding = even.HWEncoding;
  let SubRegs = [even, odd];
  let SubRegIndices = [subreg_l64, subreg_h64];
}

foreach Index = [0, 2, 4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24, 26, 28, 30] in {
  def DQ#!srl(Index, 1) :
    DQPR<!cast<FPR>("F"#Index), !cast<FPR>("F"#!add(Index, 1)), "f"#Index>;
}

def F8PRC : RegisterClass<"PPC", [f128], 128, (add (sequence "DQ%u",  0, 6),
                                                   (sequence "DQ%u", 15, 7))>;
def PPCRegQFRCAsmOperand : AsmOperandClass {
  let Name = "RegF8PRC"; let PredicateMethod = "isRegNumber";
}
def f8prc : RegisterOperand<F8PRC> {
  let ParserMatchClass = PPCRegQFRCAsmOperand;
}
nemanjai edited edge metadata.Mar 9 2016, 9:03 AM

I think we probably want to ensure we emit these instructions only when targeting Power 9 processors. So we should probably have some "Requires" in here for things that will be defined on Power9 and later only. Sure it doesn't matter now, but when we start adding SDAG patterns to these, we will be able to emit them on older processors, won't we?

cycheng updated this revision to Diff 50274.Mar 10 2016, 7:02 AM
cycheng edited edge metadata.
cycheng marked 2 inline comments as done.

Fixed issues pointed out by nemanjai (http://reviews.llvm.org/D17885#369199)

  • Implement lnia (extended mnemonic of addpcis)
  • Implement floating register pair (but we need to write inline assembly code to test register allocation, later todo)
  • Move X_BF3_IM6_RS5 to PPCInstrFormats.td

Agree, something like Requires<[HasP9]>.

def FeatureP9 : SubtargetFeature<"power9", "HasP9", "true",
                                        "Enable POWER9 instructions",
                                        [FeatureP9Vector, Power9SpecificFeatures]>;

I agree that it should be rather general like your suggestion of the power9 feature. However, I really think we shouldn't focus this on the chip that implements the ISA but the ISA version itself. That way, other chips that implement the ISA do not have to pretend they're Power 9 chips. However, I don't feel all that strongly about this so I'd like some of the others to comment on this as well (maybe @hfinkel or @kbarton).

In review for the atomic instructions (http://reviews.llvm.org/D18032) I implemented what I thought might make sense, but in any case, we should come to a consensus.

amehsan edited edge metadata.Mar 10 2016, 10:22 AM

@nemanjai If I understand correctly you are saying a better name for P8Vector would have used ISA version instead of P8 in the feature name. Is that correct?

I am asking because now I think for my implementation of P9 direct move, I may need to change the feature name from P9 to something that includes ISA version.

@nemanjai If I understand correctly you are saying a better name for P8Vector would have used ISA version instead of P8 in the feature name. Is that correct?

I don't think we need to change existing names even though I feel that referencing the chip in the feature name can be misleading. But when the feature is simply called "FeatureP9", I think that's a bit misleading if any other chips end up implementing ISA 3.0.
I think for specific subfeatures such as the direct moves, it is OK to continue to use the names we have been using. Then the ISA 3.0 feature (whatever we name it) will imply all of these Power 9 features.

kbarton edited edge metadata.EditedMar 29 2016, 3:32 PM

@nemanjai If I understand correctly you are saying a better name for P8Vector would have used ISA version instead of P8 in the feature name. Is that correct?

I don't think we need to change existing names even though I feel that referencing the chip in the feature name can be misleading. But when the feature is simply called "FeatureP9", I think that's a bit misleading if any other chips end up implementing ISA 3.0.
I think for specific subfeatures such as the direct moves, it is OK to continue to use the names we have been using. Then the ISA 3.0 feature (whatever we name it) will imply all of these Power 9 features.

I agree with @nemanjai. Please use the ISA3.0 feature, from D18032 to guard these instructions.

kbarton added inline comments.Mar 29 2016, 7:32 PM
lib/Target/PowerPC/PPCInstr64Bit.td
1285

You can define both the base and the dot versions of a class at once. Look at the definition of Form_11R in PPCInstrInfo.td for an example.

cycheng updated this revision to Diff 52327.Mar 31 2016, 11:19 PM
cycheng edited edge metadata.
cycheng marked an inline comment as done.
  • Use ISA3.0 feature to guard new 3.0 instructions
  • combine XS_RS5_RA5_SH5 and XS_RS5_RA5_SH5o into one single multi-class
kbarton accepted this revision.Apr 1 2016, 12:36 PM
kbarton edited edge metadata.

LGTM

This revision is now accepted and ready to land.Apr 1 2016, 12:36 PM
cycheng closed this revision.Apr 5 2016, 6:55 PM

Committed r265505