This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Implement Vector Multiply High/Divide Extended Builtins in LLVM/Clang
ClosedPublic

Authored by amyk on Jun 25 2020, 4:12 PM.

Details

Summary

This patch implements the function prototypes vec_mulh and vec_dive in order to utilize
the vector multiply high (vmulh[s|u][w|d]) and vector divide extended (vdive[s|u][w|d])
instructions introduced in Power10.

Depends on D82576.

Diff Detail

Event Timeline

amyk created this revision.Jun 25 2020, 4:12 PM
anil9 added a subscriber: anil9.Jun 25 2020, 4:28 PM
anil9 added inline comments.
clang/test/CodeGen/builtins-ppc-p10vector.c
139

I may be wrong but where are the variables declared ? I do not see the variables delclared above in the file, i mean many of them.

llvm/test/CodeGen/PowerPC/p10-vector-divide.ll
52

nit : you put a comment right at this position in the multiply.ll file, for consistency you could add one here or remove the one there.

amyk marked 2 inline comments as done.Jun 25 2020, 5:16 PM
amyk added inline comments.
clang/test/CodeGen/builtins-ppc-p10vector.c
139

I accidentally uploaded the wrong diff, I will update this.

llvm/test/CodeGen/PowerPC/p10-vector-divide.ll
52

Will fix.

amyk updated this revision to Diff 273578.Jun 25 2020, 7:03 PM

Addressed Anil's comments.

amyk marked 2 inline comments as done.Jun 25 2020, 7:04 PM
amyk updated this revision to Diff 274680.Jun 30 2020, 9:21 PM
amyk edited the summary of this revision. (Show Details)

Rebase patch, remove MC tests from this patch.

lei added inline comments.Jul 8 2020, 12:24 PM
clang/test/CodeGen/builtins-ppc-p10vector.c
137

why does the ck stops matching at the first param? Shouldn't we check the remaining param type and number of param are correct as well?

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
1212

nit: indent to match up with v4i32 on the previous line.

amyk marked 2 inline comments as done.Jul 8 2020, 12:42 PM
amyk added inline comments.
clang/test/CodeGen/builtins-ppc-p10vector.c
137

Yes, thanks for pointing that out. Will be fixing the CHECKs.

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
1212

I will update with the proper indentation.

amyk retitled this revision from [PowerPC][Power10] Implement Vector Multiply High/Divide Extended Builtins in LLVM/Clang to [PowerPC] Implement Vector Multiply High/Divide Extended Builtins in LLVM/Clang.Jul 27 2020, 3:43 PM
amyk edited the summary of this revision. (Show Details)
amyk updated this revision to Diff 281080.Jul 27 2020, 4:28 PM

Addressed the following comments:

  • updated CHECK lines to check for the full intrinsic call
  • updated indentation
NeHuang added inline comments.
llvm/test/CodeGen/PowerPC/p10-vector-divide.ll
59

nit: do we also need _intrinsic in the name as the test cases for the vector multiply high intrinsics.

amyk added inline comments.Aug 17 2020, 7:32 PM
llvm/test/CodeGen/PowerPC/p10-vector-divide.ll
59

I am thinking that it may be OK to leave the vector divide extended tests without the _intrinsic suffix.

The reason why I added _intrinsic to vector multiply high is because the vector multiply high instructions can be produced in two ways:

  • by the mulhs node (which is present in the test, I have named them test_vmulh...)
  • by the @llvm.ppc.altivec.vmulh[s|u][w|d] intrinsic (these tests have the _intrinsic suffix)

So far for the vector divide extended, we produce the instructions only if we have the intrinsic.

However I do not feel too strongly either way on this so if there is a preference to add _intrinsic to these tests, I can definitely do that.

lei accepted this revision as: lei.Aug 26 2020, 1:32 PM

LGTM
Thx!

This revision is now accepted and ready to land.Aug 26 2020, 1:32 PM
This revision was landed with ongoing or failed builds.Aug 26 2020, 9:14 PM
This revision was automatically updated to reflect the committed changes.