Page MenuHomePhabricator

[OpenMP] TargetLibraryInfo from "declare simd".
Needs ReviewPublic

Authored by fpetrogalli on Nov 30 2016, 2:21 AM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

This patch generates a list of global external variables that are
passed to the llvm::TargetLibraryInfo (TLI) to enable the vectorization of
loops containing calls to function that are marked with a #pragma omp
declare simd.

To do so, the global variables are generated as "global external", so
that they get removed in the middle-end and generated no useless code.

A new method of the TLI (provided in a separate LLVM patch) looks for
such global variables and updates the lists of vectorizable functions
that the vectorizer can use.

This behavior enables the programmer to provide vector routines that
are recognized as vectorizable by using the OpenMP directive "declare
simd" as follow:

$> clang -fopenmp -c -o whatever.o -O3 file.c

where file.c is

#pragma omp declare simd
double f(double x);

void aaa(double *x, double *y, int N) {
  for (int i = 0; i < N; ++i) {
    x[i] = f(y[i]);
  }
}

Diff Detail

Event Timeline

fpetrogalli retitled this revision from to [OpenMP] TargetLibraryInfo from "declare simd"..
fpetrogalli updated this object.
fpetrogalli added a reviewer: cfe-commits.
fhahn added a subscriber: fhahn.Nov 30 2016, 2:40 AM
fhahn added inline comments.Nov 30 2016, 4:30 AM
lib/CodeGen/CGOpenMPRuntime.cpp
6521

Stray space

6522

Stray space

6615

Stray space in & CGM

6662

Missing space around / in Data.VecRegSize/ VLEN.getExtValue()

6676

Is the !! intentional?

6687

Is the !! intentional?

6739

Two spaces after =

ABataev requested changes to this revision.Nov 30 2016, 4:49 AM
ABataev edited edge metadata.
  1. Please provide full context for your changes (check this document how to do this http://llvm.org/docs/Phabricator.html).
  2. Use clang-format for all your changes.
  3. Do not use backed infrastructure to mangle the name of the new function. Instead, you should build clang-based declaration and use clang infrastructure to ge

Also, I don't like the idea of the generation of real function profiles in the frontend. It was decided, that the frontend only provides a list of mangled names for vector functions, which must be generated in the backend. But you're trying something different. If you're going to change the whole solution you should send an RFC to the community

This revision now requires changes to proceed.Nov 30 2016, 4:49 AM
fpetrogalli edited edge metadata.

Rebase plus apply clang format.

ABataev requested changes to this revision.Nov 30 2016, 7:04 AM
ABataev edited edge metadata.

You did just some minor changes. The main questions are still unanswered.

This revision now requires changes to proceed.Nov 30 2016, 7:04 AM
fpetrogalli marked 5 inline comments as done.Nov 30 2016, 7:05 AM

I updated the comments related to code formatting.

Hi! Thanks for your review!

  1. Please provide full context for your changes (check this document how to do this http://llvm.org/docs/Phabricator.html).

I missed that out, should be done now.

  1. Use clang-format for all your changes.

Done.

  1. Do not use backed infrastructure to mangle the name of the new function. Instead, you should build clang-based declaration and use clang infrastructure to ge

The second part of the sentence is missing. I guess you are saying I should use the infrastructure for name mangling in clang in llvm? Is that what you mean? I think that the right place to keep something that need to be shared between LLVM and clang is the LLVM codebase, not conversely. Can we wait for other people feedback before deciding here?

Also, I don't like the idea of the generation of real function profiles in the frontend. It was decided, that the frontend only provides a list of mangled names for vector functions, which must be generated in the backend.

I am just generating a set of global variable with external linkage that are deleted by LLVM before any code generation happens. The globals are used just to populate the TargetLibraryInfoImpl class instance created in BackendUtils.cpp. See the high level description of the functionality I have provided in the RFC mail (see [1] below).I see this mechanisms being easier rather than getting a name, de-mangle it, interpret the parameters section, and then generate whatever information the vectorizer need to vectorize a particular scalar function in a loop. Instead of introducing new structures for matching what the vectorizer need swith what the TLI provides, I just use what llvm already has, the FunctionType class.

This also avoid the complications of having to write a demangle function for the vector names for each architecture, which might have completely different names across architectures and ABI versions. With this FunctionType mechanisms, ABI changes are transparent to vectorizer.

But you're trying something different. If you're going to change the whole solution you should send an RFC to the community

I've sent out an email, apologies for not specifying RFC in the subject. You can find a copy of it with the RFC in the mailing list now [1].

[1] http://lists.llvm.org/pipermail/llvm-dev/2016-November/107677.html

You did just some minor changes. The main questions are still unanswered.

Hopefully now they are! :)

Francesco

Hi! Thanks for your review!

  1. Please provide full context for your changes (check this document how to do this http://llvm.org/docs/Phabricator.html).

I missed that out, should be done now.

  1. Use clang-format for all your changes.

Done.

  1. Do not use backed infrastructure to mangle the name of the new function. Instead, you should build clang-based declaration and use clang infrastructure to ge

The second part of the sentence is missing. I guess you are saying I should use the infrastructure for name mangling in clang in llvm? Is that what you mean? I think that the right place to keep something that need to be shared between LLVM and clang is the LLVM codebase, not conversely. Can we wait for other people feedback before deciding here?

Yes, that's what I meant. Clang supports both Microsoft and Itanium-based manglings and in clang you should use clang-based solution.

Also, I don't like the idea of the generation of real function profiles in the frontend. It was decided, that the frontend only provides a list of mangled names for vector functions, which must be generated in the backend.

I am just generating a set of global variable with external linkage that are deleted by LLVM before any code generation happens. The globals are used just to populate the TargetLibraryInfoImpl class instance created in BackendUtils.cpp. See the high level description of the functionality I have provided in the RFC mail (see [1] below).I see this mechanisms being easier rather than getting a name, de-mangle it, interpret the parameters section, and then generate whatever information the vectorizer need to vectorize a particular scalar function in a loop. Instead of introducing new structures for matching what the vectorizer need swith what the TLI provides, I just use what llvm already has, the FunctionType class.

It does not matter. You should discuss it before trying to implement, especially if another solution was already accepted and implemented. Xinmin already answered you, please sync with him.

This also avoid the complications of having to write a demangle function for the vector names for each architecture, which might have completely different names across architectures and ABI versions. With this FunctionType mechanisms, ABI changes are transparent to vectorizer.

But you're trying something different. If you're going to change the whole solution you should send an RFC to the community

I've sent out an email, apologies for not specifying RFC in the subject. You can find a copy of it with the RFC in the mailing list now [1].

[1] http://lists.llvm.org/pipermail/llvm-dev/2016-November/107677.html

fpetrogalli removed a subscriber: fhahn.

I have removed all reviewer, this is not intended for code review.

I have updated the name mangling of the associated vector function.

As requested in the RFC thread, I now use a C++ vector mangling that conforms to the approved C++ name mangling standard (on Itanium, I believe).

Vector function names are now in the form: ZVGQN2v_originalname, ZVGDN4v_originalname, where "D" and "Q" maps to NEON 64-bit registers and NEON 128-bits registers respectively.

I have added tests that show the full vectorization process, from C code to assembly. As I have already said, this algorithm is name mangling independent, which means that, even if I have changed the name mangling, there was no need to update the relative llvm patch: https://reviews.llvm.org/D27249

I have updated the patch to use the name mangling scheme we agreed - compatible with the GCC name mangling scheme.

We have choose letter "n" to identify the AdvancedSIMD vector extension of A64.

The code still interfaces with the TargetLibraryInfo: I understand this is not how the community has agreed to implement the "pragma omp declare simd functionality", but I am nevertheless updating this patch just to clarify the agreement around the name mangling of vector functions.