Page MenuHomePhabricator

Add InjectTLIMappings pass to new pass manager
ClosedPublic

Authored by masoud.ataei on Feb 28 2020, 8:07 AM.

Details

Summary

This pass is created in https://reviews.llvm.org/rGd6de5f12d485a85504bc99d384a85634574a27e2 and tested for new and legacy pass manager but never added to new pass manager pipeline. I am adding it to new pass manager pipeline.

This pass is get used in Vector Function Database (VFDatabase) and without this pass in new pass manager pipeline, none of the vector libraries are working with new pass manager.

Related passes:
https://reviews.llvm.org/rG66c120f02560ef528a60924104ead66f330190f1
https://reviews.llvm.org/D74944

Diff Detail

Event Timeline

masoud.ataei created this revision.Feb 28 2020, 8:07 AM
masoud.ataei edited the summary of this revision. (Show Details)Feb 28 2020, 8:13 AM

The change LGTM, how have it been tested?

Hi @masoud.ataei , thank you for taking care of this.

Does your patch fix this bug report? https://bugs.llvm.org/show_bug.cgi?id=44770

If so, can you mention it in the commit message and update the bug report too?

In term of testing, I think that the code in bug report could be used to show that vectorization happens with the new pass manager too?

Kind regards,

Francesco

Since the InjectTLIMappings pass impacts mappings of scalar math functions to their vector counterparts in all three of the currently supported vector libraries, I would like to suggest adding a test case for each one of them. In other words, testing in the presence of -fveclib=Accelerate, -fveclib=SVML, and -fveclib=MASSV libraries.

Since the InjectTLIMappings pass impacts mappings of scalar math functions to their vector counterparts in all three of the currently supported vector libraries, I would like to suggest adding a test case for each one of them. In other words, testing in the presence of -fveclib=Accelerate, -fveclib=SVML, and -fveclib=MASSV libraries.

Isn't the follwing test already doing that via the -passes=inject-tli-mappings option?

rapet01@man-08:~/projects/upstream-clang/llvm-project (master)$ cat llvm/test/Transforms/Util/add-TLI-mappings.ll
; RUN: opt -vector-library=SVML       -inject-tli-mappings        -S < %s | FileCheck %s  --check-prefixes=COMMON,SVML
; RUN: opt -vector-library=SVML       -passes=inject-tli-mappings -S < %s | FileCheck %s  --check-prefixes=COMMON,SVML
; RUN: opt -vector-library=MASSV      -inject-tli-mappings        -S < %s | FileCheck %s  --check-prefixes=COMMON,MASSV
; RUN: opt -vector-library=MASSV      -passes=inject-tli-mappings -S < %s | FileCheck %s  --check-prefixes=COMMON,MASSV
; RUN: opt -vector-library=Accelerate -inject-tli-mappings        -S < %s | FileCheck %s  --check-prefixes=COMMON,ACCELERATE
; RUN: opt -vector-library=Accelerate -passes=inject-tli-mappings -S < %s | FileCheck %s  --check-prefixes=COMMON,ACCELERATE

...

I guess all we need to do then is to make sure that the pass is executed in the new pass manager pipelines, which is what this patch is doing.

My bad, vectorization is what needs to be tested, not the generation of the attribute.

Although, what should really be tested, is the fact that calls decorated with the attribute are in fact vectorized, which is unrelated to the fact that the attribute is attached to a MASSV or Accelerate function call.

@Whitney @fpetrogalli @pjeeva01:
Thank you guys for those comments. About the test, as @fpetrogalli said test for attribute generation is available llvm/test/Transforms/Util/add-TLI-mappings.ll for new pass manager. And I added the test to see if it is invoked in pipeline here in this patch. As @pjeeva01 said, I need to add a test to see if the vectorized function is called in presence of the vector library option.

@fpetrogalli:
About that bug report, yes this fixes that bug.

$ cat veclib.c 
void foo(float *a) {
  for (int i = 0; i != 1024; ++i)
    a[i] = __builtin_expf(a[i]);
}

$ clang -O3 -S -o - veclib.c -fveclib=Accelerate -fexperimental-new-pass-manager|grep vexpf
	bl vexpf
	bl vexpf
	bl vexpf
	bl vexpf
	bl vexpf
	bl vexpf
	bl vexpf
	bl vexpf
	bl vexpf
	bl vexpf
	bl vexpf
	bl vexpf
fpetrogalli accepted this revision.Feb 28 2020, 11:05 AM

As @pjeeva01 said, I need to add a test to see if the vectorized function is called in presence of the vector library option.

These tests are already there, those are the tests that use the -vector-library option:

LoopVectorize/X86/svml-calls.ll:; RUN: opt -vector-library=SVML -loop-vectorize -force-vector-width=4 -force-vector-interleave=1 -mattr=avx -S < %s | FileCheck %s
LoopVectorize/X86/veclib-calls.ll:; RUN: opt < %s -vector-library=Accelerate -loop-vectorize -S | FileCheck %s
LoopVectorize/X86/TLI-to-vfabi-attribute.ll~:; RUN: opt -vector-library=SVML -loop-vectorize -force-vector-width=4 -force-vector-interleave=1 -mattr=avx -S < %s | FileCheck %s
LoopVectorize/X86/svml-calls-finite.ll:; RUN: opt -vector-library=SVML -loop-vectorize -S < %s | FileCheck %s
LoopVectorize/PowerPC/widened-massv-call.ll:; RUN: opt < %s -vector-library=MASSV -force-vector-interleave=1 \
LoopVectorize/PowerPC/massv-nobuiltin.ll:; RUN: opt -vector-library=MASSV -loop-vectorize -force-vector-interleave=1 -S < %s | FileCheck %s
LoopVectorize/PowerPC/massv-unsupported.ll:; RUN: opt -vector-library=MASSV -loop-vectorize -force-vector-interleave=1 -S < %s | FileCheck %s
LoopVectorize/PowerPC/massv-altivec.ll:; RUN: opt -vector-library=MASSV -loop-vectorize -force-vector-interleave=1 -mattr=-altivec -S < %s | FileCheck %s
LoopVectorize/PowerPC/massv-calls.ll:; RUN: opt -vector-library=MASSV -loop-vectorize -force-vector-interleave=1 -S < %s | FileCheck %s

As the tests are showing, we already know that the library calls are inserted when vectorizing: InjectTLIMappings is adding the attribute vector-function-abi-variant to the call sites, and the LoopVectorizer is using it for vectorization.

What is missing in this picture (LoopVectorize with vector-function-abi-variant attribute) are the test that make sure vectorization happens when -vector-library is not used and only the attribute is present. That deserves a separate patch that is on my todo list. Of course, anyone is _very_ welcome to come up with a patch, which I happily review! :)

Thank you, this LGTM now.

This revision is now accepted and ready to land.Feb 28 2020, 11:05 AM
jsji set the repository for this revision to rG LLVM Github Monorepo.Mar 2 2020, 8:27 AM
jsji updated this revision to Diff 255338.Apr 6 2020, 7:41 AM
jsji retitled this revision from Add InjectTLIMappings pass to new pass manager to Add InjectTLIMappings pass to new pass manager.
jsji added a subscriber: jsji.

Rebased.

This revision was automatically updated to reflect the committed changes.