[cfe-commits] r85324 - in /cfe/trunk/lib: AST/ExprConstant.cpp CodeGen/CGBuiltin.cpp

[Daniel Dunbar]

Hi Mike,

On Tue, Oct 27, 2009 at 3:09 PM, Mike Stump <mrs at apple.com> wrote:
> Author: mrs
> Date: Tue Oct 27 17:09:17 2009
> New Revision: 85324
>
> URL: http://llvm.org/viewvc/llvm-project?rev=85324&view=rev
> Log:
> __builtin_object_size refinements.  Ensure we handle expressions with
> side-effects up front, as when we switch to the llvm intrinsic call
> for __builtin_object_size later, it will have two evaluations.

I find this confusing, we now have two notions of "has side effects"
in ExprConstant.cpp. Do these need to be distinct? At the least I'm
surprised the visitor doesn't reuse the EvalResult one.

Also, please include some test cases which exercise this stuff.

> We also finish off the intrinsic version of the code so we can just
> turn it on once llvm has the intrinsic.
>
> Modified:
>    cfe/trunk/lib/AST/ExprConstant.cpp
>    cfe/trunk/lib/CodeGen/CGBuiltin.cpp
>
> Modified: cfe/trunk/lib/AST/ExprConstant.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ExprConstant.cpp?rev=85324&r1=85323&r2=85324&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/AST/ExprConstant.cpp (original)
> +++ cfe/trunk/lib/AST/ExprConstant.cpp Tue Oct 27 17:09:17 2009
> @@ -151,6 +151,66 @@
>   return Result;
>  }
>
> +namespace {
> +class VISIBILITY_HIDDEN HasSideEffect
> +  : public StmtVisitor<HasSideEffect, bool> {
> +  EvalInfo &Info;
> +public:
> +
> +  HasSideEffect(EvalInfo &info) : Info(info) {}
> +
> +  // Unhandled nodes conservatively default to having side effects.
> +  bool VisitStmt(Stmt *S) {
> +    return true;
> +  }
> +
> +  bool VisitParenExpr(ParenExpr *E) { return Visit(E->getSubExpr()); }
> +  bool VisitDeclRefExpr(DeclRefExpr *E) {
> +    if (E->getType().isVolatileQualified())
> +      return true;
> +    return false;
> +  }
> +  // We don't want to evaluate BlockExprs multiple times, as they generate
> +  // a ton of code.
> +  bool VisitBlockExpr(BlockExpr *E) { return true; }
> +  bool VisitPredefinedExpr(PredefinedExpr *E) { return false; }
> +  bool VisitCompoundLiteralExpr(CompoundLiteralExpr *E)
> +    { return Visit(E->getInitializer()); }
> +  bool VisitMemberExpr(MemberExpr *E) { return Visit(E->getBase()); }
> +  bool VisitIntegerLiteral(IntegerLiteral *E) { return false; }
> +  bool VisitFloatingLiteral(FloatingLiteral *E) { return false; }
> +  bool VisitStringLiteral(StringLiteral *E) { return false; }
> +  bool VisitCharacterLiteral(CharacterLiteral *E) { return false; }
> +  bool VisitSizeOfAlignOfExpr(SizeOfAlignOfExpr *E) { return false; }
> +  bool VisitArraySubscriptExpr(ArraySubscriptExpr *E)
> +    { return Visit(E->getLHS()) && Visit(E->getRHS()); }

This should be || ?

> +  bool VisitChooseExpr(ChooseExpr *E)
> +    { return Visit(E->getChosenSubExpr(Info.Ctx)); }
> +  bool VisitCastExpr(CastExpr *E) { return Visit(E->getSubExpr()); }
> +  bool VisitBinAssign(BinaryOperator *E) { return true; }
> +  bool VisitCompoundAssign(BinaryOperator *E) { return true; }
> +  bool VisitBinaryOperator(BinaryOperator *E) { return false; }

This needs to visit the sub expressions?

> +  bool VisitUnaryPreInc(UnaryOperator *E) { return true; }
> +  bool VisitUnaryPostInc(UnaryOperator *E) { return true; }
> +  bool VisitUnaryPreDec(UnaryOperator *E) { return true; }
> +  bool VisitUnaryPostDec(UnaryOperator *E) { return true; }
> +  bool VisitUnaryDeref(UnaryOperator *E) {
> +    if (E->getType().isVolatileQualified())
> +      return true;
> +    return false;
> +  }

So does this?

> +  bool VisitUnaryOperator(UnaryOperator *E) { return Visit(E->getSubExpr()); }
> +};
> +
> +bool HasSideEffects(const Expr* E, ASTContext &Ctx) {
> +  Expr::EvalResult Result;
> +  EvalInfo Info(Ctx, Result);
> +
> +  return HasSideEffect(Info).Visit(const_cast<Expr*>(E));
> +}
> +
> +} // end anonymous namespace
> +
>  //===----------------------------------------------------------------------===//
>  // LValue Evaluation
>  //===----------------------------------------------------------------------===//
> @@ -893,12 +953,12 @@
>           }
>         }
>
> -    if (Base.HasSideEffects) {
> +    if (HasSideEffects(E->getArg(0), Info.Ctx)) {
>       if (E->getArg(1)->EvaluateAsInt(Info.Ctx).getZExtValue() < 2)
>         return Success(-1, E);
>       return Success(0, E);
>     }
> -
> +
>     return Error(E->getLocStart(), diag::note_invalid_subexpr_in_ice, E);
>   }
>
>
> Modified: cfe/trunk/lib/CodeGen/CGBuiltin.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGBuiltin.cpp?rev=85324&r1=85323&r2=85324&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/CodeGen/CGBuiltin.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGBuiltin.cpp Tue Oct 27 17:09:17 2009
> @@ -203,11 +203,13 @@
>  #if 0
>     // We pass this builtin onto the optimizer so that it can
>     // figure out the object size in more complex cases.
> -    Value *F = CGM.getIntrinsic(Intrinsic::objectsize, 0, 0);
> -    Builder.CreateCall2(F,
> -                        EmitScalarExpr(E->getArg(0)));
> -                        EmitScalarExpr(E->getArg(1)));
> -    return RValue::get(Address);
> +    const llvm::Type *ResType[] = {
> +      ConvertType(E->getType())
> +    };
> +    Value *F = CGM.getIntrinsic(Intrinsic::objectsize, ResType, 1);
> +    return RValue::get(Builder.CreateCall2(F,
> +                                           EmitScalarExpr(E->getArg(0)),
> +                                           EmitScalarExpr(E->getArg(1))));
>  #else
>     // FIXME: Implement. For now we just always fail and pretend we
>     // don't know the object size.
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>