This is an archive of the discontinued LLVM Phabricator instance.

[PPC64] Add 64-bit Vector Integer Arithmetic Instructions
ClosedPublic

Authored by kbarton on Feb 27 2015, 1:40 PM.

Details

Summary

Add the following 64-bit vector integer arithmetic instructions added in POWER8:

  • vaddudm
  • vsubudm
  • vmulesw
  • vmulosw
  • vmuleuw
  • vmulouw
  • vmuluwm
  • vmaxsd
  • vmaxud
  • vminsd
  • vminud
  • vcmpequd
  • vcmpequd.
  • vcmpgtsd
  • vcmpgtsd.
  • vcmpgtud
  • vcmpgtud.
  • vrld
  • vsld
  • vsrd
  • vsrad

Diff Detail

Event Timeline

kbarton updated this revision to Diff 20891.Feb 27 2015, 1:40 PM
kbarton retitled this revision from to [PPC64] Add 64-bit Vector Integer Arithmetic Instructions.
kbarton updated this object.
kbarton edited the test plan for this revision. (Show Details)
kbarton added reviewers: hfinkel, wschmidt, seurer, nemanjai.
kbarton added a subscriber: Unknown Object (MLST).

I have a corresponding patch to Clang to add builtins for most of these instructions. However, it is dependant on D7235 (add support for vector bool long long), which hasn't been completed yet.
I can either:

  1. Wait for D7235 to be committed
  2. Include the changes from D7235 in the patch I submit to Clang

I'm not sure which solution is preferred.

hfinkel edited edge metadata.Feb 27 2015, 1:52 PM

I have a corresponding patch to Clang to add builtins for most of these instructions. However, it is dependant on D7235 (add support for vector bool long long), which hasn't been completed yet.
I can either:

  1. Wait for D7235 to be committed

Please finish D7235, and then add the builtins as follow-up work.

  1. Include the changes from D7235 in the patch I submit to Clang

I'm not sure which solution is preferred.

Couple of inline nit comments. I'll let someone else review the substance.

-eric

lib/Target/PowerPC/PPCISelLowering.cpp
576–577

Whitespace.

7036

Personally I prefer plain text father than comments with variable names.

wschmidt edited edge metadata.Mar 2 2015, 8:10 AM

Looks like you added some vector compares as well. That should be called out in the description.

Additional comments inline.

include/llvm/IR/IntrinsicsPowerPC.td
102

You and Nemanja will have a conflict here -- first to commit wins. ;)

lib/Target/PowerPC/PPCISelLowering.cpp
7039

Hm. Nonetheless we probably want to fail gracefully if somebody writes an IR test that uses this builtin with -mattr=-p8altivec. If you can convince me we will fail gracefully before getting here under those circumstances, I'm ok with it, but otherwise an assert is probably in order.

lib/Target/PowerPC/PPCSchedule.td
376

Question for Hal: Why do we have this table of comments that needs to be updated as new instructions are added? Not sure whether it provides value, and it's an extra bit of noise in the process. Just curious. Others have noted that this list is not currently complete in any case. I'd personally rather just see it go away.

test/CodeGen/PowerPC/vec_add_sub_doubleword.ll
40

Please add this to the README.ALTIVEC file if you haven't already.

kbarton updated this revision to Diff 21031.Mar 2 2015, 12:30 PM
kbarton updated this object.
kbarton edited edge metadata.

Added Subtarget parameter to the interface to getAltivecCompareInfo to test whether the doubleword compare intrinsics are available on the current subtarget.

Updated README_ALTIVEC.txt with a FIXME from the vec_add_sub_doubleword.ll testcase.

Other than style issues called out inline, and the open question about PPCSchedule.td for Hal, this LGTM.

lib/Target/PowerPC/PPCISelLowering.cpp
576–577

You haven't fixed echristo's whitespace comment. (No space between the ! and the target of the bang.)

7053

Style: No braces around a single statement.

7067

No braces.

7086

No braces.

7095

Etc. I'll stop now... ;)

Couple inline comments.

-eric

lib/Target/PowerPC/PPCISelLowering.cpp
581–590

Also maybe unify these above?

7035

"Subtarget" or "STI" are the more common spellings.

kbarton updated this revision to Diff 21110.Mar 3 2015, 9:23 AM

Addressed style comments.

wschmidt accepted this revision.Mar 3 2015, 10:05 AM
wschmidt edited edge metadata.

LGTM. Go ahead with the PPCSchedule.td change for now. Please discuss with Hal when he returns from his travel. Thanks!

This revision is now accepted and ready to land.Mar 3 2015, 10:05 AM
kbarton closed this revision.Mar 3 2015, 11:58 AM

Committed revision 231115.

lib/Target/PowerPC/PPCISelLowering.cpp
576–577

Is this in reference to the space between ! and Subtarget?
I fixed this just now, so it will be in the next diff or the patch that I checkin if no other diffs are required.

hfinkel added inline comments.Mar 3 2015, 7:35 PM
lib/Target/PowerPC/PPCSchedule.td
376

I don't know; this table pre-dates my involvement with LLVM. I agree, however, that the table is incomplete, and likely adds little value. Feel free to remove it.

Commentary removed from PPCSchedule.td in r231246.