[cfe-commits] r127425 - /cfe/trunk/lib/Sema/SemaChecking.cpp

[Chris Lattner]

On Mar 10, 2011, at 12:03 PM, Ted Kremenek wrote:

> Author: kremenek
> Date: Thu Mar 10 14:03:42 2011
> New Revision: 127425
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=127425&view=rev
> Log:
> Profiling showed that 'CheckImplicitConversions' was very slow because of the call to getSpellingLoc().  On 'aes.c'
> in the LLVM test suite, this function was consuming 7.4% of -fsyntax-only time.  This change fixes this issue
> by delaying the check that the warning would be issued within a system macro by as long as possible.  The
> main negative of this change is now the logic for this check is done in multiple places in this function instead
> of just in one place up front.

Nice catch Ted.  Why are we checking the spelling location in any case?  Whether a token came from a system header doesn't sound that interesting to me, and *is* very expensive to compute.

-Chris

> 
> Modified:
>    cfe/trunk/lib/Sema/SemaChecking.cpp
> 
> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=127425&r1=127424&r2=127425&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Mar 10 14:03:42 2011
> @@ -2762,6 +2762,11 @@
>   return ValueInRange.toString(10);
> }
> 
> +static bool isFromSystemMacro(Sema &S, SourceLocation loc) {
> +  SourceManager &smgr = S.Context.getSourceManager();
> +  return loc.isMacroID() && smgr.isInSystemHeader(smgr.getSpellingLoc(loc));
> +}
> +  
> void CheckImplicitConversion(Sema &S, Expr *E, QualType T,
>                              SourceLocation CC, bool *ICContext = 0) {
>   if (E->isTypeDependent() || E->isValueDependent()) return;
> @@ -2771,11 +2776,12 @@
>   if (Source == Target) return;
>   if (Target->isDependentType()) return;
> 
> -  // If the conversion context location is invalid or instantiated
> -  // from a system macro, don't complain.
> -  if (CC.isInvalid() ||
> -      (CC.isMacroID() && S.Context.getSourceManager().isInSystemHeader(
> -                           S.Context.getSourceManager().getSpellingLoc(CC))))
> +  // If the conversion context location is invalid don't complain.
> +  // We also don't want to emit a warning if the issue occurs from the
> +  // instantiation of a system macro.  The problem is that 'getSpellingLoc()'
> +  // is slow, so we delay this check as long as possible.  Once we detect
> +  // we are in that scenario, we just return.
> +  if (CC.isInvalid())
>     return;
> 
>   // Never diagnose implicit casts to bool.
> @@ -2784,8 +2790,11 @@
> 
>   // Strip vector types.
>   if (isa<VectorType>(Source)) {
> -    if (!isa<VectorType>(Target))
> +    if (!isa<VectorType>(Target)) {
> +      if (isFromSystemMacro(S, CC))
> +        return;
>       return DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_vector_scalar);
> +    }
> 
>     Source = cast<VectorType>(Source)->getElementType().getTypePtr();
>     Target = cast<VectorType>(Target)->getElementType().getTypePtr();
> @@ -2793,8 +2802,12 @@
> 
>   // Strip complex types.
>   if (isa<ComplexType>(Source)) {
> -    if (!isa<ComplexType>(Target))
> +    if (!isa<ComplexType>(Target)) {
> +      if (isFromSystemMacro(S, CC))
> +        return;
> +
>       return DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_complex_scalar);
> +    }
> 
>     Source = cast<ComplexType>(Source)->getElementType().getTypePtr();
>     Target = cast<ComplexType>(Target)->getElementType().getTypePtr();
> @@ -2822,13 +2835,19 @@
>             return;
>         }
> 
> +        if (isFromSystemMacro(S, CC))
> +          return;
> +
>         DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_float_precision);
>       }
>       return;
>     }
> 
> -    // If the target is integral, always warn.
> +    // If the target is integral, always warn.    
>     if ((TargetBT && TargetBT->isInteger())) {
> +      if (isFromSystemMacro(S, CC))
> +        return;
> +      
>       Expr *InnerE = E->IgnoreParenImpCasts();
>       if (FloatingLiteral *LiteralExpr = dyn_cast<FloatingLiteral>(InnerE)) {
>         DiagnoseImpCast(S, LiteralExpr, T, CC,
> @@ -2852,6 +2871,9 @@
>     // TODO: this should happen for bitfield stores, too.
>     llvm::APSInt Value(32);
>     if (E->isIntegerConstantExpr(Value, S.Context)) {
> +      if (isFromSystemMacro(S, CC))
> +        return;
> +
>       std::string PrettySourceValue = Value.toString(10);
>       std::string PrettyTargetValue = PrettyPrintInRange(Value, TargetRange);
> 
> @@ -2863,6 +2885,10 @@
> 
>     // People want to build with -Wshorten-64-to-32 and not -Wconversion
>     // and by god we'll let them.
> +    
> +    if (isFromSystemMacro(S, CC))
> +      return;
> +    
>     if (SourceRange.Width == 64 && TargetRange.Width == 32)
>       return DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_integer_64_32);
>     return DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_integer_precision);
> @@ -2871,6 +2897,10 @@
>   if ((TargetRange.NonNegative && !SourceRange.NonNegative) ||
>       (!TargetRange.NonNegative && SourceRange.NonNegative &&
>        SourceRange.Width == TargetRange.Width)) {
> +        
> +    if (isFromSystemMacro(S, CC))
> +      return;
> +
>     unsigned DiagID = diag::warn_impcast_integer_sign;
> 
>     // Traditionally, gcc has warned about this under -Wsign-compare.
> @@ -2893,9 +2923,13 @@
>            SourceEnum->getDecl()->getTypedefForAnonDecl()) &&
>           (TargetEnum->getDecl()->getIdentifier() ||
>            TargetEnum->getDecl()->getTypedefForAnonDecl()) &&
> -          SourceEnum != TargetEnum)
> +          SourceEnum != TargetEnum) {
> +        if (isFromSystemMacro(S, CC))
> +          return;
> +
>         return DiagnoseImpCast(S, E, T, CC, 
>                                diag::warn_impcast_different_enum_types);
> +      }
> 
>   return;
> }
> 
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits