Page MenuHomePhabricator

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

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



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


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
1467 ↗(On Diff #192443)

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
1467 ↗(On Diff #192443)

@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.
1467 ↗(On Diff #192443)

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.

1467 ↗(On Diff #192443)

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
76 ↗(On Diff #194193)

How about the finite versions? eg: __expf_finite

1 ↗(On Diff #194193)

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

4 ↗(On Diff #194193)

How about powerpc (32 bit) or powerpc64?

93 ↗(On Diff #194193)

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)

282 ↗(On Diff #194193)

Missing some intrinsics test? @sqrt_f64_intrinsic?@sqrt_f32_intrinsic?

1524 ↗(On Diff #194193)

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?

1525 ↗(On Diff #194193)

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,

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.

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.

282 ↗(On Diff #194193)

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

The LLVM portion of this patch committed in

Thank you @nemanjai.