r233227 - Reapply r232888 after applying a fix for -msse4 code generation.

[Eric Christopher]

Hi Duncan,

Sorry for the delay, I took a while to think about this in context and
agree that it's probably the best way to deal with this. Have the front end
be very explicit in what it's giving to the back end rather than try to be
concise. FWIW until the patch about migrating target attributes to
constructors/destructors this wouldn't have worked - there are a few other
places I think are missing, but for now:

dzur:~/sources/llvm/tools/clang> git svn dcommit
Committing to https://llvm.org/svn/llvm-project/cfe/trunk ...
M lib/CodeGen/CGCall.cpp
M test/CodeGen/function-target-features.c
Committed r235936

-eric

On Tue, Mar 31, 2015 at 8:59 PM Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > On 2015 Mar 25, at 16:14, Eric Christopher <echristo at gmail.com> wrote:
> >
> > Author: echristo
> > Date: Wed Mar 25 18:14:47 2015
> > New Revision: 233227
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=233227&view=rev
> > Log:
> > Reapply r232888
>
> This is awesome, btw :).
>
> > after applying a fix for -msse4 code generation.
> >
> > As a note, any target that uses fake target features via command
> > line options will have similar problems.
> >
> > Added:
> >    cfe/trunk/test/CodeGen/function-target-features.c
> > Modified:
> >    cfe/trunk/lib/CodeGen/CGCall.cpp
> >
> > Modified: cfe/trunk/lib/CodeGen/CGCall.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCall.cpp?rev=233227&r1=233226&r2=233227&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/lib/CodeGen/CGCall.cpp (original)
> > +++ cfe/trunk/lib/CodeGen/CGCall.cpp Wed Mar 25 18:14:47 2015
> > @@ -31,6 +31,7 @@
> > #include "llvm/IR/InlineAsm.h"
> > #include "llvm/IR/Intrinsics.h"
> > #include "llvm/Transforms/Utils/Local.h"
> > +#include <sstream>
> > using namespace clang;
> > using namespace CodeGen;
> >
> > @@ -1475,6 +1476,26 @@ void CodeGenModule::ConstructAttributeLi
> >
> >     if (!CodeGenOpts.StackRealignment)
> >       FuncAttrs.addAttribute("no-realign-stack");
> > +
> > +    // Add target-cpu and target-features work if they differ from the
> defaults.
> > +    std::string &CPU = getTarget().getTargetOpts().CPU;
> > +    if (CPU != "" && CPU != getTarget().getTriple().getArchName())
>
> Why not the following?
>
>     if (CPU != "")
>
> The frontend and backend don't necessarily have the same default CPU.
> IMO that's a bug that should be fixed eventually, but I think we
> should encode the CPU to ensure that it matches up on the other side.
>
> > +      FuncAttrs.addAttribute("target-cpu",
> getTarget().getTargetOpts().CPU);
>
> You can reuse the `CPU` local here.
>
> > +
> > +    // TODO: FeaturesAsWritten gets us the features on the command line,
> > +    // for canonicalization purposes we might want to avoid putting
> features
> > +    // in the target-features set if we know it'll be one of the default
> > +    // features in the backend, e.g. corei7-avx and +avx.
>
> Same point as above for target-cpu.  I don't think we should rely on
> defaults matching in the backend until the defaults come from the same
> codebase.
>
> > +    std::vector<std::string> &Features =
> > +        getTarget().getTargetOpts().FeaturesAsWritten;
> > +    if (!Features.empty()) {
>
> Even here, shouldn't we just encode `"target-features"=""` when
> Features is empty?
>
> > +      std::stringstream S;
> > +      std::copy(Features.begin(), Features.end(),
> > +                std::ostream_iterator<std::string>(S, ","));
> > +      // The drop_back gets rid of the trailing space.
> > +      FuncAttrs.addAttribute("target-features",
> > +                             StringRef(S.str()).drop_back(1));
> > +    }
> >   }
> >
> >   ClangToLLVMArgMapping IRFunctionArgs(getContext(), FI);
> >
> > Added: cfe/trunk/test/CodeGen/function-target-features.c
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/function-target-features.c?rev=233227&view=auto
> >
> ==============================================================================
> > --- cfe/trunk/test/CodeGen/function-target-features.c (added)
> > +++ cfe/trunk/test/CodeGen/function-target-features.c Wed Mar 25
> 18:14:47 2015
> > @@ -0,0 +1,21 @@
> > +// This test verifies that we produce target-cpu and target-features
> attributes
> > +// on functions when they're different from the standard cpu and have
> written
> > +// features.
> > +
> > +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s
> -target-feature +avx | FileCheck %s -check-prefix=AVX-FEATURE
> > +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s
> -target-feature +avx | FileCheck %s -check-prefix=AVX-NO-CPU
> > +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s
> -target-feature +avx512f -target-feature +avx512er | FileCheck %s
> -check-prefix=TWO-AVX
> > +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s
> -target-cpu corei7 | FileCheck %s -check-prefix=CORE-CPU
> > +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s
> -target-cpu corei7 -target-feature +avx | FileCheck %s
> -check-prefix=CORE-CPU-AND-FEATURES
> > +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s
> -target-cpu x86-64 | FileCheck %s -check-prefix=X86-64-CPU-NOT
> > +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s
> -target-cpu corei7-avx -target-feature -avx | FileCheck %s
> -check-prefix=AVX-MINUS-FEATURE
> > +
> > +void foo() {}
> > +
> > +// AVX-FEATURE: "target-features"="+avx"
> > +// AVX-NO-CPU-NOT: target-cpu
> > +// TWO-AVX: "target-features"="+avx512f,+avx512er"
> > +// CORE-CPU: "target-cpu"="corei7"
> > +// CORE-CPU-AND-FEATURES: "target-cpu"="corei7" "target-features"="+avx"
> > +// X86-64-CPU-NOT: "target-cpu"
> > +// AVX-MINUS-FEATURE: "target-features"="-avx"
> >
> >
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150427/d137dc56/attachment.html>