Page MenuHomePhabricator

Add fveclib option and add several entries to table of vectorizable functions.
ClosedPublic

Authored by mzolotukhin on Mar 5 2015, 8:01 PM.

Details

Summary

Add fveclib option.

Partially fill the list of vectorizable functions for framework
Accelerate.

This is the last, sixth, of 6 patches for enabling vectorization of calls.

Diff Detail

Event Timeline

mzolotukhin updated this revision to Diff 21329.Mar 5 2015, 8:01 PM
mzolotukhin retitled this revision from to Add fveclib option and add several entries to table of vectorizable functions..
mzolotukhin updated this object.
mzolotukhin edited the test plan for this revision. (Show Details)
mzolotukhin added a subscriber: Unknown Object (MLST).
hfinkel added inline comments.Mar 5 2015, 8:56 PM
lib/CodeGen/BackendUtil.cpp
253

I think that I'd still rather add the functions to TLI inside of TLI. I like the idea that the frontend can add others, and we should keep that capability. However, there is no reason that other frontends should not be able to take advantage of this knowledge. We should translate the Clang-level CodeGenOpt into LLVM-level CodeGenOpts, and then add these there.

lib/Driver/Tools.cpp
2854

You don't need this if.

Furthermore, I think that this whole thing can be replaced by:

Args.AddLastArg(CmdArgs, options::OPT_fveclib);
mzolotukhin updated this revision to Diff 21411.Mar 6 2015, 5:19 PM
Address Hal's remarks:
- Simplify arg passing.
- Move initialization of the vectorizable-function tables into TLI.

Hi Hal,
I moved the tables initialization to TLI, the corresponding TLI patch is here:
http://reviews.llvm.org/D8131
Now frontends can either use addVectorizableFunctionsFromVecLib to initialize the tables for a known library, or manually fill the table and use addVectorizableFunctions. Did I get it right, that it's what you wanted to keep?

hfinkel added inline comments.Mar 8 2015, 2:29 PM
include/clang/Frontend/CodeGenOptions.def
164

All of the entries here have a descriptive comment above them, please add one here as well.

include/clang/Frontend/CodeGenOptions.h
51

Use -> Use the

lib/Frontend/CompilerInvocation.cpp
375

I'd rather we actually validated the value here, instead of just mapping all unknown strings to 'NoLibrary'. I'd rather something like this:

if (Arg *A = Args.getLastArg(OPT_fveclib)) {
  if (A->getValue() == "Accelerate")
    Opts.setVecLib(CodeGenOptions::Accelerate);
  else if (A->getValue() == "none")
    Opts.setVecLib(CodeGenOptions::NoLibrary);
  else
    Diags.Report(diag::err_drv_invalid_value)
      << A->getAsString(Args) << A->getValue();
}
hfinkel edited edge metadata.Mar 8 2015, 5:21 PM
  • Original Message -----

From: "Michael Zolotukhin" <mzolotukhin@apple.com>
To: mzolotukhin@apple.com, hfinkel@anl.gov, aschwaighofer@apple.com, nrotem@apple.com, "james molloy"
<james.molloy@arm.com>
Cc: cfe-commits@cs.uiuc.edu
Sent: Friday, March 6, 2015 7:25:52 PM
Subject: Re: [PATCH] Add fveclib option and add several entries to table of vectorizable functions.

Hi Hal,
I moved the tables initialization to TLI, the corresponding TLI patch
is here:
http://reviews.llvm.org/D8131
Now frontends can either use addVectorizableFunctionsFromVecLib to
initialize the tables for a known library, or manually fill the
table and use addVectorizableFunctions. Did I get it right, that
it's what you wanted to keep?

I think this makes sense.

-Hal

http://reviews.llvm.org/D8097

EMAIL PREFERENCES

http://reviews.llvm.org/settings/panel/emailpreferences/
mzolotukhin updated this revision to Diff 21540.Mar 9 2015, 6:06 PM
mzolotukhin edited edge metadata.
  • Report unrecognized value in fveclib CL option.

This LGTM, but needs a regression test for the command-line handling before it can be committed (that we accept -fveclib=Accelerate and -fveclib=none, that the last one wins, and that we get an error on an unknown one). There are lots of potential examples, see test/Driver/code-model.c, test/Driver/visibility.cpp.

include/clang/Driver/Options.td
761

Remove blank line.

  • Add regression tests for '-fveclib' option.
  • Remove blank line.
hfinkel accepted this revision.Mar 11 2015, 12:27 AM
hfinkel edited edge metadata.

As per our IRC discussion, setting the linking flags for the Accelerate library when provided -fveclib=Accelerate will be added in follow-up. This LGTM.

This revision is now accepted and ready to land.Mar 11 2015, 12:27 AM
mzolotukhin closed this revision.Mar 17 2015, 1:07 PM

Thanks, committed in r232533.