From cfe-dev Mon Aug 03 17:56:51 2015 From: Justin Bogner Date: Mon, 03 Aug 2015 17:56:51 +0000 To: cfe-dev Subject: Re: [cfe-dev] [PROPOSAL] Reintroduce guards for Intel intrinsic headers Message-Id: X-MARC-Message: https://marc.info/?l=cfe-dev&m=143862538111677 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--=-=-=" --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Eric Christopher writes: > Good question, I'm working on the diagnostics and what I think should hap= pen. > 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 wr= ote: > > Would cherrypicking the diagnostics to the 3.7 branch be better or wo= rse? > (I'm of two minds, curious what others think...) >=20=20=20=20 > On Sun, Aug 2, 2015 at 12:00 PM Justin Bogner > wrote: >=20=20=20=20 > Eric Christopher writes: > > On Thu, Jul 30, 2015 at 10:12 AM Reid Kleckner > wrote: > > > >=C2=A0 =C2=A0 =C2=A0I'm opposed to this. Going forward, I would = really like target > intrinsics > >=C2=A0 =C2=A0 =C2=A0to be available regardless of the current fe= ature set, so users > don't need > >=C2=A0 =C2=A0 =C2=A0hacks like these. > > > > Agreed. > > =C2=A0 > > > >=C2=A0 =C2=A0 =C2=A0I see two ways to do this with different tra= deoffs: > >=C2=A0 =C2=A0 =C2=A01. Diagnose missing target attributes when c= alling the intel > intrinsics. I > >=C2=A0 =C2=A0 =C2=A0was surprised to find that we don't already = do this. > > > > Sorry. This is on my list of things to do. >=20=20=20=20=20=20=20=20 > +hans >=20=20=20=20=20=20=20=20 > I agree with the direction of moving to use target attributes ins= tead > 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. > > =C2=A0 > > I think we should revert this on the 3.7 branch. It can stay as i= s on > trunk assuming the diagnostics are coming soon. > > The type of warning we generate is an interesting question, I'll handle t= his > at the bottom. > =C2=A0 > > 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: >=20=20=20=20=20=20=20=20 > =C2=A0 https://llvm.org/bugs/show_bug.cgi?id=3D24125 > =C2=A0 https://llvm.org/bugs/show_bug.cgi?id=3D24087 > =C2=A0 https://llvm.org/bugs/show_bug.cgi?id=3D24335 > > 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. Basical= ly > they had a diagnostics test go awry. The last one is definitely a regress= ion > in diagnostic quality, but not bad code. > =C2=A0 > > 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. > =C2=A0 > > 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 th= rough > how we do the warning is interesting though. > > Let's take gcc as an example: > > #include > > int foo(__m256i a) { > =C2=A0 return _mm256_extract_epi32(a, 3); > } > extern =C2=A0__m256i a; > int bar() { > =C2=A0 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 re= port > That, so -mavx) to compile: > > In file included from .../immintrin.h:41:0, > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0from foo.c:= 1: > foo.c:4:10: error: '__builtin_ia32_vextractf128_si256' needs isa option -= m32 > =C2=A0 =C2=A0return _mm256_extract_epi32(a, 3); > > which is a little weird, but ok. > > Oddly enough this doesn't warn: > > #include > > int __attribute__((target("avx"))) foo(__m256i a) { > =C2=A0 return _mm256_extract_epi32(a, 3); > } > extern =C2=A0__m256i a; > int bar() { > =C2=A0 return foo(a); > } > > whereas I'd think it probably should. Essentially gcc only warns in the c= ase > 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 refactorin= g. > 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. --=-=-= Content-Type: text/x-patch Content-Disposition: attachment; filename=target-warning.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, 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 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()) { + SmallVector 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()) + 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). --=-=-= Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ cfe-dev mailing list cfe-dev@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev --=-=-=--