This is an archive of the discontinued LLVM Phabricator instance.

Lower generic MASSV entries to PowerPC subtarget-specific entries
ClosedPublic

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

Details

Summary

This patch (second of two patches) lowers 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 first patch 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 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:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2019, 8:09 AM
pjeeva01 updated this revision to Diff 211165.Jul 22 2019, 12:15 PM

Re-basing this patch off of the patch in https://reviews.llvm.org/D59881

jsji added a comment.Jul 29 2019, 2:58 PM

Why we need a new pass to do this simple lowering?
Why can't we do it similar to InitLibcalls and change the suffixes for subtarget using setLibcallName?

llvm/test/CodeGen/PowerPC/lower-massv.ll
2

You can use -check-prefixes=CHECK-PWR9,CHECK instead of two -check-prefix

3

Add -verify-machineinstrs to all RUN lines please.

7

Remove 'datalyout` and triple, we use triple in RUN line now.

78

We normally don't rely on function attribute to distinguish test.
Can we test them in different RUN lint with different -mattr?

jsji requested changes to this revision.Aug 27 2019, 7:48 PM

Move out of review queue, need author's action.

This revision now requires changes to proceed.Aug 27 2019, 7:48 PM
pjeeva01 added a comment.EditedAug 28 2019, 8:00 AM

Why we need a new pass to do this simple lowering?
Why can't we do it similar to InitLibcalls and change the suffixes for subtarget using setLibcallName?

A separate pass for MASSV lowering is worthwhile because it allows better separation of concerns.
In the future, if need be, other PPC sub-target specific lowering decisions for MASSV may be encapsulated in this pass too.

pjeeva01 updated this revision to Diff 217660.EditedAug 28 2019, 8:56 AM

Addressed review comments in this diff:

  1. use -mattr instead of relying attributes on the caller; separate the tests to a separate .ll file
  2. add -verify-machineinstrs to the run line
  3. combine multiple check-prefix into one check-prefixes
  4. remove datalayout and triple from the .ll file
pjeeva01 marked 4 inline comments as done.Aug 28 2019, 8:58 AM
jsji added a comment.Aug 28 2019, 10:28 AM

A separate pass for MASSV lowering is worthwhile because it allows better separation of concerns.
In the future, if need be, other PPC sub-target specific lowering decisions for MASSV may be encapsulated in this pass too.

What is the benefits other than allows better separation of concerns?
I don't see this compelling reason to be a pass, especially considering the unnecessary compile time it will cost.

A separate pass for MASSV lowering is worthwhile because it allows better separation of concerns.
In the future, if need be, other PPC sub-target specific lowering decisions for MASSV may be encapsulated in this pass too.

What is the benefits other than allows better separation of concerns?
I don't see this compelling reason to be a pass, especially considering the unnecessary compile time it will cost.

  1. *_massv functions serve as place holder for math functions during loop vectorization. They are not actual entries in the MASS library. I think lowering them to actual entries in the MASS library deserves a separate pass rather than polluting the TargetLowering.cpp, which is already big as it is.
  1. If we were to use setLibcallName, we would need to repeat the list of all supported MASS entries in TargetLowering as well. With the current structure, we are reusing the list in VecFuncs.def. Also, this structure sits well with vectorization of math functions with other libraries too, namely: SVML and Accelerate.
jsji added a comment.Aug 28 2019, 11:55 AM
  1. *_massv functions serve as place holder for math functions during loop vectorization. They are not actual entries in the MASS library. I think lowering them to actual entries in the MASS library deserves a separate pass rather than polluting the TargetLowering.cpp, which is already big as it is.

Why this has to be in TargetLowering.cpp, they can be in PPCISelLowering.cpp.

  1. If we were to use setLibcallName, we would need to repeat the list of all supported MASS entries in TargetLowering as well. With the current structure, we are reusing the list in VecFuncs.def. Also, this structure sits well with vectorization of math functions with other libraries too, namely: SVML and Accelerate.

Why we can't reusing the list in VecFuncs.def with setLibcallName?

  1. *_massv functions serve as place holder for math functions during loop vectorization. They are not actual entries in the MASS library. I think lowering them to actual entries in the MASS library deserves a separate pass rather than polluting the TargetLowering.cpp, which is already big as it is.

Why this has to be in TargetLowering.cpp, they can be in PPCISelLowering.cpp.

  1. If we were to use setLibcallName, we would need to repeat the list of all supported MASS entries in TargetLowering as well. With the current structure, we are reusing the list in VecFuncs.def. Also, this structure sits well with vectorization of math functions with other libraries too, namely: SVML and Accelerate.

Why we can't reusing the list in VecFuncs.def with setLibcallName?

Yes, the list in VecFuncs.def could be reused in PPCISelLowering. But, PPCISelLowering.cpp is already big as it is. Adding more functions to handle this lowering will only add to its size and a list of features it already supports. Therefore, my preference is to separate out the MASSV lowering functionality into a separate transformation.

jsji added a comment.Aug 29 2019, 7:05 AM

Yes, the list in VecFuncs.def could be reused in PPCISelLowering. But, PPCISelLowering.cpp is already big as it is. Adding more functions to handle this lowering will only add to its size and a list of features it already supports. Therefore, my preference is to separate out the MASSV lowering functionality into a separate transformation.

Files too large is NOT a good reason to sacrifice compile time here. We can always refactor the implementation to reduce the size of file.
However, it will take far more effort to figure out the unnecessary compile time increase and fix it in the future.

@jsji Using setLibcallName to override the generic fun*_massv names with target specific names is feasible. However, it entails extension of ISDNodes to support vector types as well. Furthermore, with the current infrastructure, distinguishing MASSV calls from a list of scalar math, and other runtime library calls will incur the overheads of look up and comparison.

Given that the setLibcallName approach to lowering massv calls involves non-trivial extension to both target-independent and PPC-specific files, and also incurs the aforementioned overhead, I believe a separate pass for lowering is a reasonable approach.

I have addressed rest of the issues pointed out in the last review.

jsji accepted this revision.Oct 31 2019, 2:26 PM

Given that the setLibcallName approach to lowering massv calls involves non-trivial extension to both target-independent and PPC-specific files,

OK, yes, it is a non-trivial work.

LGTM for now considering the amount of work needed for setLibcallName.

llvm/lib/Target/PowerPC/PPCLowerMASSVEntries.cpp
110

FCache? Typo?

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

@jsji @nemanjai I dont have commit access, could you please commit this patch for me? Thanks.

jsji added a comment.Nov 4 2019, 7:10 AM

@jsji @nemanjai I dont have commit access, could you please commit this patch for me? Thanks.

OK. I will commit it for you.

This revision was automatically updated to reflect the committed changes.