Page MenuHomePhabricator

[Vector] Pass VectLib to LTO backend so TLI build correct vector function list
AbandonedPublic

Authored by wenlei on Apr 4 2020, 9:56 PM.

Details

Summary

-fveclib switch not propagated to LTO backends, and as a result, LTO populates vector list as if no math lib is used. This change fixed the driver to lld, and to backend propagation of -fveclib setting.

Diff Detail

Unit TestsFailed

TimeTest
250 msClang Tools.clang-tidy/checkers::fuchsia-multiple-inheritance.cpp
Script: -- : 'RUN: at line 1'; /usr/bin/python /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang-tools-extra/test/../test/clang-tidy/check_clang_tidy.py /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang-tools-extra/test/clang-tidy/checkers/fuchsia-multiple-inheritance.cpp fuchsia-multiple-inheritance /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/tools/clang/tools/extra/test/clang-tidy/checkers/Output/fuchsia-multiple-inheritance.cpp.tmp
240 msLLVM.Other::opt-bisect-legacy-pass-manager.ll
Script: -- : 'RUN: at line 19'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/opt -O3 -opt-bisect-limit=0 < /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm/test/Other/opt-bisect-legacy-pass-manager.ll | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/llc -O3 -opt-bisect-limit=0

Event Timeline

wenlei created this revision.Apr 4 2020, 9:56 PM
Herald added a project: Restricted Project. · View Herald Transcript

Added usual lld and LTO side reviewers. I am just a bit worried that option name changes like this patch and D77231 could accidentally slip through if I did not react in time...

We're trying to move towards encoding all of this in the IR. And in fact, I recently implemented a series of patches to make the TLI to be built per-function, and along with some patches from @gchatelet to encode -fno-builtin* as function attributes, we now handle that part of the TLII with IR. See D67923 which is the last patch in the series. We should encode the vectlib as function attributes similarly, and just thread that through to the TLI.

wenlei added a comment.EditedApr 5 2020, 10:02 AM

We're trying to move towards encoding all of this in the IR. And in fact, I recently implemented a series of patches to make the TLI to be built per-function, and along with some patches from @gchatelet to encode -fno-builtin* as function attributes, we now handle that part of the TLII with IR. See D67923 which is the last patch in the series. We should encode the vectlib as function attributes similarly, and just thread that through to the TLI.

Thanks for quick look, @tejohnson. Yeah, making vectlib a function attribute would be more consistent with other the way other TLI attributes are handled. However, unlike some other attributes, which really need to be per-function, conceptually, vectlib setting won't be different from function to function in a module. Implementation-wise, even though TLI is now built per-function, there's a BaselineInfoImpl within each TargetLibraryInfo, that's still shared among functions, and VectorDescs which is populated based on vectlib setting belongs to the shared baseline. We could also move VectorDescs and its population into per-function part of TLI, but I felt that's a bit overkill, though I don't have a strong opinion on this. So I followed how other module level stuff like lto-sample-profile is passed to backend..

Alternatively, we could chose to always run InjectTLIMappings in pre-LTO pipeline, even if we don't run vectorizer pre-LTO for ThinLTO, so we always have per-function attribute vector-function-abi-variant populated and threaded through to LTO time vectorizer. We will have to make sure the vector function list in TLI (based on vectlib setting) has no other use case though.

(More on TLI, while this change (or a variant of this) enables vectorization that uses math library for ThinLTO w/ legacy PM, there's separate issue with new PM as InjectTLIMappings is not scheduled before Vectorization for LTO pipeline. I will send a separate patch for that once we settle on a good way to handle this one.)

hoyFB added a comment.Apr 5 2020, 10:31 AM

We're trying to move towards encoding all of this in the IR. And in fact, I recently implemented a series of patches to make the TLI to be built per-function, and along with some patches from @gchatelet to encode -fno-builtin* as function attributes, we now handle that part of the TLII with IR. See D67923 which is the last patch in the series. We should encode the vectlib as function attributes similarly, and just thread that through to the TLI.

That's an interesting idea. How does the linkage work if two functions have different vectlib attributes? Linking against two vectlibs may cause name conflicts or other issues.

We're trying to move towards encoding all of this in the IR. And in fact, I recently implemented a series of patches to make the TLI to be built per-function, and along with some patches from @gchatelet to encode -fno-builtin* as function attributes, we now handle that part of the TLII with IR. See D67923 which is the last patch in the series. We should encode the vectlib as function attributes similarly, and just thread that through to the TLI.

That's an interesting idea. How does the linkage work if two functions have different vectlib attributes? Linking against two vectlibs may cause name conflicts or other issues.

Actually this may be best encoded in a module metadata that with an error merging type, so that conflicting options get flagged. What happens today in a non-LTO build if conflicting veclib options are provided to different TUs?

Linking against two vectlibs may cause name conflicts or other issues.

Of all three supported match libraries, all functions from Accelerate are prefixed with v; all MASS library functions are suffixed with _massv; and all SVML functions are prefixed with __svml_. See VecFuncs.def. So at least the exported functions don't have name conflicts.

What happens today in a non-LTO build if conflicting veclib options are provided to different TUs?

I think it will work, as long as used math libraries are all provided to linker. Even if not, I'm not sure if this is something we want to actively prevent from the use of fvectlib switch. fvectlib controls codegen/optimizer, and whether the resulting codegen can leads to linking problem (if any), is kind of orthogonal, and probably no different from a regular linking/symbol resolution problem if there're conflicts in the libraries provide.

Linking against two vectlibs may cause name conflicts or other issues.

Of all three supported match libraries, all functions from Accelerate are prefixed with v; all MASS library functions are suffixed with _massv; and all SVML functions are prefixed with __svml_. See VecFuncs.def. So at least the exported functions don't have name conflicts.

Ok then it does sound like these could be handled on a per-function basis, similar to how -fno-builtin* are handled. I.e. a function attribute to indicate the veclib, which would then be naturally preserved during LTO even after merging/importing across modules. Similar to how -fno-builtin* are handled, these would need to be examined when inlining (see the new TargetLibraryInfo::areInlineCompatible). Presumably we would want to block inlining between functions with different veclib attributes in the LTO backends.

What happens today in a non-LTO build if conflicting veclib options are provided to different TUs?

I think it will work, as long as used math libraries are all provided to linker. Even if not, I'm not sure if this is something we want to actively prevent from the use of fvectlib switch. fvectlib controls codegen/optimizer, and whether the resulting codegen can leads to linking problem (if any), is kind of orthogonal, and probably no different from a regular linking/symbol resolution problem if there're conflicts in the libraries provide.

ruiu added inline comments.Apr 5 2020, 10:17 PM
lld/ELF/Config.h
132

We name variables after their corresponding command line flags, so this should be ltoVectorLibrary.

grimar added inline comments.Apr 6 2020, 2:43 AM
lld/ELF/Options.td
491

You need to update the documentation either (lld/docs/ld.lld.1)

wenlei added a comment.Apr 6 2020, 4:01 PM

Ok then it does sound like these could be handled on a per-function basis, similar to how -fno-builtin* are handled. I.e. a function attribute to indicate the veclib, which would then be naturally preserved during LTO even after merging/importing across modules. Similar to how -fno-builtin* are handled, these would need to be examined when inlining (see the new TargetLibraryInfo::areInlineCompatible). Presumably we would want to block inlining between functions with different veclib attributes in the LTO backends.

@tejohnson, we could do that. But then on the other hand, technically almost everything for module or whole program can be passed as a function attribute, and yet we have switches passed to backend for many of those things. Wondering what's the convention or rule (if there's one) we want to follow? Specifically, if we only use function attributes for stuff that's indeed going to be different between functions, then vectlib isn't in that category; or if we use function attributes for the greatest flexibility whenever we can, then many other things should be function attributes too (though it's essentially duplication in IR, and probably not the most efficient).

Ok then it does sound like these could be handled on a per-function basis, similar to how -fno-builtin* are handled. I.e. a function attribute to indicate the veclib, which would then be naturally preserved during LTO even after merging/importing across modules. Similar to how -fno-builtin* are handled, these would need to be examined when inlining (see the new TargetLibraryInfo::areInlineCompatible). Presumably we would want to block inlining between functions with different veclib attributes in the LTO backends.

@tejohnson, we could do that. But then on the other hand, technically almost everything for module or whole program can be passed as a function attribute, and yet we have switches passed to backend for many of those things. Wondering what's the convention or rule (if there's one) we want to follow? Specifically, if we only use function attributes for stuff that's indeed going to be different between functions, then vectlib isn't in that category; or if we use function attributes for the greatest flexibility whenever we can, then many other things should be function attributes too (though it's essentially duplication in IR, and probably not the most efficient).

Passing the option through the driver to the linker is the legacy approach. But it isn't really scalable and has other issues, so we've been moving towards having all the necessary info in the IR itself. For one, this helps deal with cases where different options were specified for different source files. For another, it keeps the same build behavior with LTO and non-LTO. I.e. for this option, if the build system specified it for the cc compiles but not the links, it would work for O2 but not for LTO if it had to be propagated via the linker. It would work for LTO if it was propagated via the IR.

Ok then it does sound like these could be handled on a per-function basis, similar to how -fno-builtin* are handled. I.e. a function attribute to indicate the veclib, which would then be naturally preserved during LTO even after merging/importing across modules. Similar to how -fno-builtin* are handled, these would need to be examined when inlining (see the new TargetLibraryInfo::areInlineCompatible). Presumably we would want to block inlining between functions with different veclib attributes in the LTO backends.

@tejohnson, we could do that. But then on the other hand, technically almost everything for module or whole program can be passed as a function attribute, and yet we have switches passed to backend for many of those things. Wondering what's the convention or rule (if there's one) we want to follow? Specifically, if we only use function attributes for stuff that's indeed going to be different between functions, then vectlib isn't in that category; or if we use function attributes for the greatest flexibility whenever we can, then many other things should be function attributes too (though it's essentially duplication in IR, and probably not the most efficient).

Passing the option through the driver to the linker is the legacy approach. But it isn't really scalable and has other issues, so we've been moving towards having all the necessary info in the IR itself. For one, this helps deal with cases where different options were specified for different source files. For another, it keeps the same build behavior with LTO and non-LTO. I.e. for this option, if the build system specified it for the cc compiles but not the links, it would work for O2 but not for LTO if it had to be propagated via the linker. It would work for LTO if it was propagated via the IR.

Makes sense, thanks for clarification. I created D77632 to make vect lib setting a per-function attribute.

wenlei abandoned this revision.Apr 12 2020, 9:56 AM