Page MenuHomePhabricator

[VFABI] TargetLibraryInfo mappings in IR.
ClosedPublic

Authored by fpetrogalli on Nov 11 2019, 6:32 PM.

Diff Detail

Event Timeline

fpetrogalli created this revision.Nov 11 2019, 6:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 11 2019, 6:32 PM

This patch is not functional yet, but I have added it here to show in the test what I intend to do.

I am adding a pass that adds the "vector-function-abi-variant" attribute from the mappings that are stored in the TargetLibraryInfo (TLI).

This patch is based on the work published in https://reviews.llvm.org/D69976 and https://reviews.llvm.org/D70089

There is not need to review the code for now, all I need to know is whether people agree on this approach.

andwar added a subscriber: andwar.Nov 12 2019, 12:47 AM

@fpetrogalli This means that all passes that need to care about vector variants of functions need to add a dependency on this pass, right? This seems quite similar to how TBAA info is loaded from IR metadata using an analysis pass, after which the information can be queried using the AliasAnalysis interfaces. This is doing a similar thing, but then for querying vector-variants of functions, so +1 for the approach!

We should not forget to document the new mechanism (the VFABI, metadata attributes and the SVFS mechanism), perhaps as a separate document. The metadata format should at least be described in the LangRef.

llvm/include/llvm/Transforms/Utils/InjectTLIMappings.h
20

Don't forget about the old pass manager :)

IIUC, this a transformation pass (it does modify the module, e.g. by appendToCompilerUsed(*M, {Global});). So you probably want to register it with one of the optimisation pipelines. I _believe_ that that's how you do it:

For legacy PM:

For the new PM, you probably want to add your pass to an existing FunctionPassPamanager, e.g.

Once that's done, your Pass will be run _automagically_ together with other passes in the pipeline. This is just a quick brain-dump so please ping me if it's unclear.

llvm/include/llvm/Transforms/Utils/InjectTLIMappings.h
20

Legacy :)

Also, I am not a fan of appending Pass to pass classes (it's clear what this class inherits from either way). Also, if you use INITIALIZE_PASS_BEGIN, Pass is going to be prepended to the _Initialize_ method anyway (so you will have initializeInjestTLIMappingPassPass): https://github.com/llvm/llvm-project/blob/848007cfbc7509543c5b8604ae063bb6c8ffa0a9/llvm/include/llvm/PassSupport.h#L62

llvm/lib/Transforms/Utils/InjectTLIMappings.cpp
105

[nit] 'things' -> 'thinks'

128

[nit] What follows is the definition of InjectTLIMappingsPass::run though.

llvm/test/Transforms/Util/add-TLI-mappings.ll
2

For this to work you need to register a command line option. Why not use print-after and print-before instead? Or maybe we do need a command line option?

sdesmalen added inline comments.Nov 12 2019, 4:05 AM
llvm/lib/Transforms/Utils/InjectTLIMappings.cpp
67

When you think the patch is ready to be reviewed, can you address the comments that I added in D69976 before you removed it?

llvm/test/Transforms/Util/add-TLI-mappings.ll
18

I don't think you need to add a loop here to prove the IR contains the vectorized versions of the IR, a call to @sin should be sufficient.

This is an update in which I have tried to add the legacy pass manager.

It is not working yet, but I think I am on a good path!

Thank you @andwar for all the pointers.

For reference, this is the linking error I am getting:

llvm-project/llvm/include/llvm/Transforms/Utils/InjectTLIMappings.h:31: error: undefined reference to 'vtable for llvm::InjectTLIMappingsLegacy'
/usr/bin/ld: the vtable symbol may be undefined because the class is missing its key function
fpetrogalli marked 5 inline comments as done.Nov 12 2019, 9:28 PM
fpetrogalli added inline comments.
llvm/include/llvm/Transforms/Utils/InjectTLIMappings.h
20

I am working on it! :)

New pass manager command line option missing.

FWIW, you can checkout D69930 for an example of adding a pass with more hookup into the system.

llvm/include/llvm/Transforms/Utils/InjectTLIMappings.h
23

I doubt you need the constructor or the name function.

llvm/include/llvm/Transforms/Utils/ModuleUtils.h
115 ↗(On Diff #229004)

Unrelated?

llvm/lib/Transforms/Utils/InjectTLIMappings.cpp
2

TODO, also below

20

Is there precedent for not having it in all lower-case letters?

75

Wasn't that checked below but with a different call to the module?

81

Where is the VarArgs restrictions checked? If it is implicit, please add an assertion that the original callee is not vararg.

93

Why do you need to query Global here? Global is VectorF isn't it?

119

For brevity and without the need to assign:
SetVector<StringRef> OriginalSetOfMappings(Mappings.begin(), Mappings.end());

134

Can we make 16 a return value of a (maybe static) function in TLI?

Style, above:

if (!...count(...))
153

I doubt you need the lllvm.

Above, auto CI*please :)

llvm/test/Transforms/Util/add-TLI-mappings.ll
2

command line options are good for various things, please make sure they work. (new and old PM)

fpetrogalli marked 15 inline comments as done.

Thank you all for the review. The pass is now fuctional, working for
the libraries supported by the TLI: SVML, Accelerate and MASSV.

I haven't added it to any of the optimization pipelines at the moment,
under the assumption that once this pass is listed in the required
passes of the loop vectorizer, it will be automatically loaded after
the TLI wrapper pass.

fpetrogalli added inline comments.Nov 13 2019, 2:31 PM
llvm/include/llvm/Transforms/Utils/ModuleUtils.h
115 ↗(On Diff #229004)

Yes, but still useful. I have also removed a group comment.

I'd rather not create a separate patch for this?

llvm/lib/Transforms/Utils/InjectTLIMappings.cpp
20

The pass debug option is all lower case now (and consequently, the command line too).

75

This avoids running the creation of the function if the function already exists. Otherwise I expect Function::Create to have some problems.

93

Ah right, good catch. Fixed.

153

I doubt you need the lllvm.

You mean I don't need to wrap the pass in the llvm namespace? I have done it in the header file too. Is that wrong?

jdoerfert added inline comments.Nov 13 2019, 3:06 PM
llvm/include/llvm/Transforms/Utils/ModuleUtils.h
115 ↗(On Diff #229004)

create an RFC one and commit it w/o review if it is trivial.

llvm/lib/Transforms/Utils/InjectTLIMappings.cpp
75

I mean you checked that already:

Function *VariantF = M->getFunction(TLIName);
if (!VariantF)
  addVariantDeclaration(CI, VF, TLIName);

but once with getFunction and once with getNamedValue. I think you don't need two checks and you should pick consistent the one lookup call you want.

93

Forgot to update or not fixed?

153

you don't need it here because you opened the namespace. you should not open namespaces in headers that is why you wrap it in the namespace there. Plus, you don't need the explicit qualification here because these are not top level declarations as you have in the header.

fpetrogalli marked 3 inline comments as done.Nov 13 2019, 8:39 PM
fpetrogalli added inline comments.
llvm/include/llvm/Transforms/Utils/ModuleUtils.h
115 ↗(On Diff #229004)
andwar added inline comments.Nov 14 2019, 4:06 AM
llvm/include/llvm/Transforms/Utils/InjectTLIMappings.h
23

What about this one? You don't need the name function, the one provided by the base class is fine: https://github.com/llvm/llvm-project/blob/848007cfbc7509543c5b8604ae063bb6c8ffa0a9/llvm/include/llvm/IR/PassManager.h#L373

As for the constructor, the auto-generated one should be sufficient here. Is it not?

llvm/lib/Transforms/Utils/InjectTLIMappings.cpp
166

[nit] `Legacy PM implementation (the pass manager is legacy, not the pass :) ).

fpetrogalli marked 13 inline comments as done.

Address last round of reviews.

fpetrogalli added inline comments.Nov 14 2019, 8:36 PM
llvm/include/llvm/Transforms/Utils/InjectTLIMappings.h
23

Contructor and name function removed. That was too much copy and paste.

llvm/lib/Transforms/Utils/InjectTLIMappings.cpp
75

I was using Global mostly to be able to call appendToCompilerUsed, which works on globals. Then, I realized that Function inherits from Global, so there is no need to use Global at all.

I also have replaced the check on having an empty body with an assertion, just in case someone modifies the function in the middle and populates the body of the function before invoking appendToCompilerUsed.

93

Not it is fixed, no more Global.

153

Facepalm myself at the using namespace llvm; on top of this cpp file.

Thanks for the explanation.

jdoerfert accepted this revision.Nov 14 2019, 8:52 PM

Some minor comments below, otherwise LGTM.

@sdesmalen any more comments?

llvm/lib/Transforms/Utils/InjectTLIMappings.cpp
121

The call is not free but invariant:
for (unsigned VF = 2, MaxVF = TLI.getWidestVF(ScalarName); VF <= MaxVF; VF *= 2) {

155

Remove Changed or doe something with it.

171

You can preserve more here, e.g. all CFG analysis.

183

I think one of the false could be a true, unclear if that will make a difference though.

This revision is now accepted and ready to land.Nov 14 2019, 8:52 PM
fpetrogalli marked 2 inline comments as done.Nov 14 2019, 9:41 PM
fpetrogalli added inline comments.
llvm/lib/Transforms/Utils/InjectTLIMappings.cpp
29

TODO: The description of the counter is misleading, only declarations are added in this pass.

140–143

TODO: remove braces.

fpetrogalli marked 5 inline comments as done.

Update according to last round of review from @jdoerfert.

Thank you.

llvm/lib/Transforms/Utils/InjectTLIMappings.cpp
183

It is not clear what the cfg parameter is for. I'll leave it false for now. If we need to revise it, we will change it later.

This revision was automatically updated to reflect the committed changes.