This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC][Power10] Implementation of 128-bit Binary Vector Mod and Sign Extend builtins
ClosedPublic

Authored by Conanap on Sep 9 2020, 9:42 AM.

Details

Reviewers
saghir
nemanjai
hfinkel
amyk
Group Reviewers
Restricted Project
Summary

This patch implements 128-bit Binary Vector Mod and Sign Extend builtins for PowerPC10.

Diff Detail

Event Timeline

Conanap created this revision.Sep 9 2020, 9:42 AM
Conanap requested review of this revision.Sep 9 2020, 9:42 AM
amyk requested changes to this revision.Sep 15 2020, 4:40 PM
amyk added a subscriber: amyk.

A question I have is, is it possible for the 128-bit vector modulo instructions be open coded?

clang/lib/Headers/altivec.h
17416

nit: Move these under the existing vec_mod builtins.
Also, is it possible for these to be open coded instead? We have vec_mod for other types that are open coded.

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

nit: Indent v1i128 underneath the top v1i128.

This revision now requires changes to proceed.Sep 15 2020, 4:40 PM
Conanap added inline comments.Sep 17 2020, 12:18 PM
clang/lib/Headers/altivec.h
17416

Probably? Although the downsteram impementation is actually not open coded. We can still change it though.

Conanap updated this revision to Diff 292901.Sep 18 2020, 2:32 PM

Open coded VMODS/UQ instead.

Conanap updated this revision to Diff 293076.Sep 20 2020, 11:26 PM

Open Coded instead

clang/lib/Headers/altivec.h
17416

Probably? Although the downsteram impementation is actually not open coded. We can still change it though.

nemanjai accepted this revision.Sep 21 2020, 3:54 AM

The nits can be addressed when committing the code. LGTM otherwise.

llvm/lib/Target/PowerPC/PPCInstrAltivec.td
1452–1461

The indentation is off on all of these. Should probably be something like:

def VEXTSB2W :
  VX_VT5_EO5_VB5<1538, 16, "vextsb2w",
                 [(set v4i32:$vD, (int_ppc_altivec_vextsb2w v16i8:$vB))]>;
llvm/test/CodeGen/PowerPC/p10-vector-sign-extend.ll
2

Nit: only one of these requires P10. The others are P9 instructions. You have split the tests for the front end changes, please split the back end test as well.

amyk accepted this revision.Sep 21 2020, 6:56 AM

Thanks for open coding. Aside from Nemanja's nits, I believe all my concerns have been addressed. LGTM.

This revision is now accepted and ready to land.Sep 21 2020, 6:56 AM
Conanap closed this revision.Sep 23 2020, 2:41 AM

Committed with Nemanja's comments addressed in the commit. Hash d7eb917a7cb793f49e16841fc24826b988dd5c8f