[prev in list] [next in list] [prev in thread] [next in thread]
List: cfe-dev
Subject: Re: [cfe-dev] [PROPOSAL] Reintroduce guards for Intel intrinsic headers
From: Justin Bogner <mail () justinbogner ! com>
Date: 2015-08-03 17:56:51
Message-ID: m2twsgi7bw.fsf () justinbogner ! com
[Download RAW message or body]
Eric Christopher <echristo@gmail.com> writes:
> Good question, I'm working on the diagnostics and what I think should happen.
> It's not directly compatible with the gcc ones and it'll be a bit tricky, but
> I think possible.
>
> Responding to Justin inline here:
>
> On Sun, Aug 2, 2015 at 5:09 PM Chandler Carruth <chandlerc@google.com> wrote:
>
> Would cherrypicking the diagnostics to the 3.7 branch be better or worse?
> (I'm of two minds, curious what others think...)
>
> On Sun, Aug 2, 2015 at 12:00 PM Justin Bogner <mail@justinbogner.com>
> wrote:
>
> Eric Christopher <echristo@gmail.com> writes:
> > On Thu, Jul 30, 2015 at 10:12 AM Reid Kleckner <rnk@google.com>
> wrote:
> >
> > I'm opposed to this. Going forward, I would really like target
> intrinsics
> > to be available regardless of the current feature set, so users
> don't need
> > hacks like these.
> >
> > Agreed.
> >
> >
> > I see two ways to do this with different tradeoffs:
> > 1. Diagnose missing target attributes when calling the intel
> intrinsics. I
> > was surprised to find that we don't already do this.
> >
> > Sorry. This is on my list of things to do.
>
> +hans
>
> I agree with the direction of moving to use target attributes instead
> of
> relying on flaky ifdefs, but without any errors or warnings here this
> is
> a pretty serious diagnostic regression.
>
> As I said in the commit that added the rest of the target support, if we can't
> code generate we will error, it's not a good error, but it's not going to
> silently generate bad code.
>
>
>
> I think we should revert this on the 3.7 branch. It can stay as is on
> trunk assuming the diagnostics are coming soon.
>
> The type of warning we generate is an interesting question, I'll handle this
> at the bottom.
>
>
> Right now we end up in spaces where we get crashes in the backend
> instead of a sensible error in far too many situations. Notably:
>
> https://llvm.org/bugs/show_bug.cgi?id=24125
> https://llvm.org/bugs/show_bug.cgi?id=24087
> https://llvm.org/bugs/show_bug.cgi?id=24335
>
> I'll point out that two of these are people from the same company with a
> similar testcase. So it's not quite as widespread as you'd think. Basically
> they had a diagnostics test go awry. The last one is definitely a regression
> in diagnostic quality, but not bad code.
>
>
> Additionally, I'm told this causes issues with configure scripts
> misdetecting available features, as well as strange compatibility
> issues
> like the one that led to this thread.
>
> Yeah, this is crap IMO. Anyone that's doing the things you mentioned is
> actually doing it wrong. A configure check that's going for a preprocessor
> warning rather than an actual compile/execute test? That's asking for some
> pain. There are better ways to do it.
>
>
> This feature is woefully incomplete. We need the warnings/errors for
> it
> to be acceptable quality.
>
> This is a bit hyperbolic, but I agree that it's a regression. I meant to get
> the warning in much sooner than this and for that I apologize. Working through
> how we do the warning is interesting though.
>
> Let's take gcc as an example:
>
> #include <immintrin.h>
>
> int foo(__m256i a) {
> return _mm256_extract_epi32(a, 3);
> }
> extern __m256i a;
> int bar() {
> return foo(a);
> }
>
> Here, we'll warn in foo for the fact that the builtin (builtin? there's no
> builtin in my code here :) needs -m32 (ok, that's just a bug, I should report
> That, so -mavx) to compile:
>
> In file included from .../immintrin.h:41:0,
> from foo.c:1:
> foo.c:4:10: error: '__builtin_ia32_vextractf128_si256' needs isa option -m32
> return _mm256_extract_epi32(a, 3);
>
> which is a little weird, but ok.
>
> Oddly enough this doesn't warn:
>
> #include <immintrin.h>
>
> int __attribute__((target("avx"))) foo(__m256i a) {
> return _mm256_extract_epi32(a, 3);
> }
> extern __m256i a;
> int bar() {
> return foo(a);
> }
>
> whereas I'd think it probably should. Essentially gcc only warns in the case
> where we'd call a builtin that required certain target specific code
> generation, however, as pr24335 shows, that's not necessarily good enough for
> us. (It might be? Should those just be code generation bugs? I mean,
> theoretically we should be able to generate code for them right?)
>
> So, taking opinions here. Honestly warning in the second case is probably much
> easier than warning in the first even if it will take a bit of refactoring.
> Warning in the first would require yet another field to the builtins on
> required ISA level perhaps? Or maybe something else.
>
> So, thoughts?
Well, I was thinking it'd be something along the lines of the attached.
We'll need to replace some BUILTINS with
TARGET_BUILTIN(ID, TYPE, ATTRS, FEATURE)
in the various Builtins*.def files, and then FEATURE can be target
attributes ("avx", "aes", etc). This way we can treat the builtins like
they have a target attribute, so we can warn correctly for the macros
and for bare uses of the builtins as well as for the higher level
functions.
The biggest issue here is that walking through the parent function's
features isn't really sufficient in cases where a feature implies other
ones. For example, I'm pretty sure a function with __target__("avx512f")
can use avx instructions.
It's also a bit unclear what we'd do if we have a builtin that requires
"either feature X or feature Y", but I don't think that really comes up
in practice.
[Attachment #3 (text/x-patch)]
diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index 43c1b47..d209102 100644
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -429,6 +429,7 @@ def warn_redecl_library_builtin : Warning<
def err_builtin_definition : Error<"definition of builtin function %0">;
def err_arm_invalid_specialreg : Error<"invalid special register for builtin">;
def err_invalid_cpu_supports : Error<"invalid cpu feature string for builtin">;
+def err_call_requires_feature : Error<"function call requires feature '%0'">;
def warn_builtin_unknown : Warning<"use of unknown builtin %0">,
InGroup<ImplicitFunctionDeclare>, DefaultError;
def warn_dyn_class_memaccess : Warning<
diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h
index 9769a03..7d4be32 100644
--- a/include/clang/Sema/Sema.h
+++ b/include/clang/Sema/Sema.h
@@ -8794,6 +8794,8 @@ private:
SourceLocation Loc, SourceRange Range,
VariadicCallType CallType);
+ bool checkTargetFeatures(CallExpr *TheCall, StringRef Features);
+
bool CheckObjCString(Expr *Arg);
ExprResult CheckBuiltinFunctionCall(FunctionDecl *FDecl,
diff --git a/lib/Sema/SemaChecking.cpp b/lib/Sema/SemaChecking.cpp
index d9d26e2..eb01b38 100644
--- a/lib/Sema/SemaChecking.cpp
+++ b/lib/Sema/SemaChecking.cpp
@@ -1028,6 +1028,18 @@ bool Sema::CheckSystemZBuiltinFunctionCall(unsigned BuiltinID,
}
bool Sema::CheckX86BuiltinFunctionCall(unsigned BuiltinID, CallExpr *TheCall) {
+ switch (BuiltinID) {
+ default:
+ return false;
+#define BUILTIN(ID, TYPE, ATTRS)
+#define TARGET_BUILTIN(ID, TYPE, ATTRS, FEATURE) \
+ case X86::BI##ID: \
+ if (checkTargetFeatures(TheCall, FEATURE)) \
+ return true; \
+ break;
+#include "clang/Basic/BuiltinsX86.def"
+ }
+
unsigned i = 0, l = 0, u = 0;
switch (BuiltinID) {
default: return false;
@@ -1352,6 +1364,29 @@ void Sema::checkCall(NamedDecl *FDecl, const FunctionProtoType *Proto,
}
}
+/// Check that the call satisfies the target attr requirements.
+bool Sema::checkTargetFeatures(CallExpr *TheCall, StringRef Features) {
+ SmallVector<StringRef, 1> SplitFeatures;
+ Features.split(SplitFeatures, ",");
+
+ for (StringRef Feature : SplitFeatures) {
+ if (Context.getTargetInfo().hasFeature(Feature))
+ continue;
+ // TODO: This isn't really sufficient for cases where a feature implies
+ // another. We may need to refactor TargetInfo to provide a way to query it.
+ if (const auto *ParentAttr = getCurFunctionDecl()->getAttr<TargetAttr>()) {
+ SmallVector<StringRef, 1> ParentFeatures;
+ ParentAttr->getFeatures().split(ParentFeatures, ",");
+ if (std::find(ParentFeatures.begin(), ParentFeatures.end(), Feature) !=
+ ParentFeatures.end())
+ continue;
+ }
+ return Diag(TheCall->getLocStart(), diag::err_call_requires_feature)
+ << Feature;
+ }
+ return false;
+}
+
/// CheckConstructorCall - Check a constructor call for correctness and safety
/// properties not enforced by the C type system.
void Sema::CheckConstructorCall(FunctionDecl *FDecl,
@@ -1387,6 +1422,9 @@ bool Sema::CheckFunctionCall(FunctionDecl *FDecl, CallExpr *TheCall,
IsMemberFunction, TheCall->getRParenLoc(),
TheCall->getCallee()->getSourceRange(), CallType);
+ if (const auto *TA = FDecl->getAttr<TargetAttr>())
+ checkTargetFeatures(TheCall, TA->getFeatures());
+
IdentifierInfo *FnInfo = FDecl->getIdentifier();
// None of the checks below are needed for functions that don't have
// simple names (e.g., C++ conversion functions).
_______________________________________________
cfe-dev mailing list
cfe-dev@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic