This is an archive of the discontinued LLVM Phabricator instance.

Initial support for vectorization using MASSV (IBM MASS vector library)
ClosedPublic

Authored by pjeeva01 on Mar 27 2019, 8:04 AM.

Details

Summary

This patch (first of two patches) enables the vectorizer to recognize the IBM MASS vector library routines. This patch specifically adds support for recognizing the “-vector-library=MASSV” option, and defines mappings from IEEE standard scalar math functions to generic PowerPC MASS vector counterparts.
For instance, the generic PowerPC MASS vector entry for double-precision “cbrt” function is “__cbrtd2_massv”

The second patch will further lower the generic PowerPC vector entries to PowerPC subtarget-specific entries.
For instance, the PowerPC generic entry “cbrtd2_massv” is lowered to “cbrtd2_P9” or Power9 subtarget.

The overall support for MASS vector library is presented as such in two patches for ease of review.

Diff Detail

Repository
rL LLVM

Event Timeline

pjeeva01 created this revision.Mar 27 2019, 8:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2019, 8:04 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
jsji added inline comments.Mar 28 2019, 2:16 PM
llvm/lib/Analysis/TargetLibraryInfo.cpp
1467

Can we split the refactoring of these definitions into llvm/Analysis/VecFuncs.def into a NFC patch first?
And leave only the MASSV related change in this patch ?

pjeeva01 added inline comments.Mar 29 2019, 6:32 AM
llvm/lib/Analysis/TargetLibraryInfo.cpp
1467

@jsji Thank you for looking into this.

@mmasten @mzolotukhin If you agree with this refactoring suggestion, I will go ahead and create an NFC patch for the same.

pjeeva01 marked an inline comment as done.Apr 3 2019, 9:27 AM
pjeeva01 added inline comments.
llvm/lib/Analysis/TargetLibraryInfo.cpp
1467

NFC patch that simply factors out these definitions into llvm/Analysis/VecFuncs.def is posted [[ llvm/Analysis/VecFuncs.def | here ]]

@pjeeva01, please post the delta assuming a base that has D60211 incorporated.

llvm/lib/Analysis/TargetLibraryInfo.cpp
1467

You mean, posted as D60211?

pjeeva01 updated this revision to Diff 194193.Apr 8 2019, 12:40 PM

Updated the patch assuming a base that has D60211 incorporated.

jsji added inline comments.Apr 12 2019, 2:53 PM
llvm/include/llvm/Analysis/VecFuncs.def
76

How about the finite versions? eg: __expf_finite

llvm/test/Transforms/LoopVectorize/PowerPC/massv-calls.ll
2

do we require any attr? eg: vsx? what happen to ppc without altivec or vsx?

5

How about powerpc (32 bit) or powerpc64?

94

It would be better if we check all the key elements of call , not only the name .
eg: call <2 x double> @__cbrtd2_massv(<2 x double> %2)

283

Missing some intrinsics test? @sqrt_f64_intrinsic?@sqrt_f32_intrinsic?

1525

Shall we add some negative tests to make sure that we don't vectorize functions not in the list, eg: arbitrary functions, function in ACCELERATE/SVML but not in MASSV?

1526

Maybe add a negative test where the nobuiltin attribute is present too.

@pjeeva01 Can you address Jinsong's comments? It would be good to add the negative tests for sure:

  • No vectorization with -mattr=-altivec
  • No vectorization with the nobuiltin attribute
  • A couple of calls to functions that MASSV doesn't have a vector equivalent for

Also, it would be good to add a run line for big endian.

pjeeva01 updated this revision to Diff 198475.EditedMay 7 2019, 8:23 AM

Thank you for your comments @jsji
This patch includes following updates:

  • negative tests for math functions lacking vector counterparts, for altivec targets, and for nobuiltin attributes.

I have left the CHECK for *_massv function calls as-is because they are intended to check whether the massv suffixed calls are generated. The subsequent patch that further lowers such *_massv calls to PowerPC subtarget-specific entries will have checks with the full function signatures.

MASS vector library is currently supported only for 64-bit Linux LE. Since the use of this vector library is option controlled, I think it's fair for users to experience link errors if and when used with 32-bit Linux, for instance.

jsji added a subscriber: chandlerc.May 14 2019, 7:59 AM

Some more nit-picker, other than that, mostly good to me.

However, as Chandler mentioned in similar patch for AArch64 in https://reviews.llvm.org/D53927,

Adding new support for a vector math library seems worth getting reviewed / approved by someone reasonably deeply familiar w/ our vectorization infra.

I would be good to have @hfinkel or @nemanjai or someone more familiar with vectorization infra to double confirm and approve it.

llvm/test/Transforms/LoopVectorize/PowerPC/massv-altivec.ll
2 ↗(On Diff #198475)

I would be great if we can add some comments to describe the overall test points, eg: this is for negative test of massv without altivec support. This apply to all testcases in this patch.

llvm/test/Transforms/LoopVectorize/PowerPC/massv-calls.ll
283

Still missing in latest patch?

pjeeva01 updated this revision to Diff 199822.EditedMay 16 2019, 7:12 AM

This patch includes following updates:

  1. sqrt (f32 and f64) intrinsics are lowered to their vector intrinsic counterparts; lowering them to massv entries is beyond the scope of this patch. Relevant tests for the sqrt intrinsics are added to massv-unsupported.ll
  2. The test files include comments to portray their intent.
nemanjai accepted this revision.May 16 2019, 2:31 PM

LGTM. Thanks for getting this done.

This revision is now accepted and ready to land.May 16 2019, 2:31 PM

I'll commit this for you soon Jeeva.

The LLVM portion of this patch committed in https://reviews.llvm.org/rL362568.

The LLVM portion of this patch committed in https://reviews.llvm.org/rL362568.

Thank you @nemanjai.