From e1cf160fe71a16ee9077d2962985f68a7c6a1dab Mon Sep 17 00:00:00 2001 From: Vaibhav Thakkar Date: Mon, 26 Feb 2024 12:41:46 +0100 Subject: [PATCH] Fix builtin macros and zero init for memory operations --- .clang-tidy | 1 + include/clad/Differentiator/CladUtils.h | 2 +- include/clad/Differentiator/Compatibility.h | 11 +++++++ lib/Differentiator/CladUtils.cpp | 33 ++++++++++++++++++--- lib/Differentiator/ReverseModeVisitor.cpp | 29 ++++++++++++++---- test/ForwardMode/Pointer.C | 2 +- test/Gradient/Pointers.C | 7 +++-- 7 files changed, 71 insertions(+), 14 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index 9c90276eb..894a98c74 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -28,6 +28,7 @@ Checks: > -readability-function-cognitive-complexity, -readability-implicit-bool-conversion, -cppcoreguidelines-avoid-magic-numbers, + -clang-analyzer-cplusplus.NewDeleteLeaks, CheckOptions: - key: readability-identifier-naming.ClassCase diff --git a/include/clad/Differentiator/CladUtils.h b/include/clad/Differentiator/CladUtils.h index 9b086ac50..56519593d 100644 --- a/include/clad/Differentiator/CladUtils.h +++ b/include/clad/Differentiator/CladUtils.h @@ -329,7 +329,7 @@ namespace clad { bool IsLiteral(const clang::Expr* E); - bool IsMemoryAllocationFunction(const clang::FunctionDecl* FD); + bool IsMemoryFunction(const clang::FunctionDecl* FD); bool IsMemoryDeallocationFunction(const clang::FunctionDecl* FD); } // namespace utils } // namespace clad diff --git a/include/clad/Differentiator/Compatibility.h b/include/clad/Differentiator/Compatibility.h index 62d71641c..9f11a652e 100644 --- a/include/clad/Differentiator/Compatibility.h +++ b/include/clad/Differentiator/Compatibility.h @@ -766,5 +766,16 @@ static inline const DeclSpec& Sema_ActOnStartOfLambdaDefinition_ScopeOrDeclSpec( ,Node->isTransparent() #endif +// Clang 9 above added isa_and_nonnull. +#if CLANG_VERSION_MAJOR < 9 +template bool isa_and_nonnull(const Y* Val) { + return Val && isa(Val); +} +#else +template bool isa_and_nonnull(const Y* Val) { + return llvm::isa_and_nonnull(Val); +} +#endif + } // namespace clad_compat #endif //CLAD_COMPATIBILITY diff --git a/lib/Differentiator/CladUtils.cpp b/lib/Differentiator/CladUtils.cpp index 3d1a872c9..d7d599ce5 100644 --- a/lib/Differentiator/CladUtils.cpp +++ b/lib/Differentiator/CladUtils.cpp @@ -6,6 +6,7 @@ #include "clang/AST/Decl.h" #include "clang/AST/Expr.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/Basic/Builtins.h" #include "clang/Basic/SourceLocation.h" #include "clang/Sema/Lookup.h" #include "llvm/ADT/SmallVector.h" @@ -400,6 +401,12 @@ namespace clad { auto& C = semaRef.getASTContext(); if (!TSI) TSI = C.getTrivialTypeSourceInfo(qType); + if (clad_compat::isa_and_nonnull(initializer)) + // If the initializer is an implicit value init expression, then + // we don't need to pass it explicitly to the CXXNewExpr. As, clang + // internally adds it when initializer is ParenListExpr and + // DirectInitRange is valid. + initializer = semaRef.ActOnParenListExpr(noLoc, noLoc, {}).get(); auto newExpr = semaRef .BuildCXXNew( @@ -642,18 +649,36 @@ namespace clad { isa(E); } - bool IsMemoryAllocationFunction(const clang::FunctionDecl* FD) { + bool IsMemoryFunction(const clang::FunctionDecl* FD) { + +#if CLANG_VERSION_MAJOR > 12 if (FD->getBuiltinID() == Builtin::BImalloc) return true; - if (FD->getBuiltinID() == Builtin::BIcalloc) + if (FD->getBuiltinID() == Builtin::ID::BIcalloc) + return true; + if (FD->getBuiltinID() == Builtin::ID::BIrealloc) + return true; + if (FD->getBuiltinID() == Builtin::ID::BImemset) + return true; +#else + if (FD->getNameAsString() == "malloc") return true; - if (FD->getBuiltinID() == Builtin::BIrealloc) + if (FD->getNameAsString() == "calloc") return true; + if (FD->getNameAsString() == "realloc") + return true; + if (FD->getNameAsString() == "memset") + return true; +#endif return false; } bool IsMemoryDeallocationFunction(const clang::FunctionDecl* FD) { - return FD->getBuiltinID() == Builtin::BIfree; +#if CLANG_VERSION_MAJOR > 12 + return FD->getBuiltinID() == Builtin::ID::BIfree; +#else + return FD->getNameAsString() == "free"; +#endif } } // namespace utils } // namespace clad diff --git a/lib/Differentiator/ReverseModeVisitor.cpp b/lib/Differentiator/ReverseModeVisitor.cpp index c0acb6f0f..a181785ca 100644 --- a/lib/Differentiator/ReverseModeVisitor.cpp +++ b/lib/Differentiator/ReverseModeVisitor.cpp @@ -1445,7 +1445,7 @@ Expr* getArraySizeExpr(const ArrayType* AT, ASTContext& context, // For calls to C-style memory allocation functions, we do not need to // differentiate the call. We just need to visit the arguments to the // function. - if (utils::IsMemoryAllocationFunction(FD)) { + if (utils::IsMemoryFunction(FD)) { for (const Expr* Arg : CE->arguments()) { StmtDiff ArgDiff = Visit(Arg, dfdx()); CallArgs.push_back(ArgDiff.getExpr()); @@ -2649,8 +2649,7 @@ Expr* getArraySizeExpr(const ArrayType* AT, ASTContext& context, bool isPointerType = VD->getType()->isPointerType(); bool isInitializedByNewExpr = false; // Check if the variable is pointer type and initialized by new expression - if (isPointerType && (VD->getInit() != nullptr) && - isa(VD->getInit())) + if (isPointerType && VD->getInit() && isa(VD->getInit())) isInitializedByNewExpr = true; // VDDerivedInit now serves two purposes -- as the initial derivative value @@ -2842,7 +2841,20 @@ Expr* getArraySizeExpr(const ArrayType* AT, ASTContext& context, if (m_ExternalSource) m_ExternalSource->ActBeforeFinalizingDifferentiateSingleStmt(direction::reverse); - addToCurrentBlock(SDiff.getStmt_dx(), direction::reverse); + // If the statement is a standalone call to a memory function, we want to + // add its derived statement in the same block as the original statement. + // For ex: memset(x, 0, 10) -> memset(_d_x, 0, 10) + Stmt* stmtDx = SDiff.getStmt_dx(); + bool dxInForward = false; + if (auto* callExpr = dyn_cast_or_null(stmtDx)) + if (auto* FD = dyn_cast(callExpr->getCalleeDecl())) + if (utils::IsMemoryFunction(FD)) + dxInForward = true; + + if (dxInForward) + addToCurrentBlock(stmtDx, direction::forward); + else + addToCurrentBlock(SDiff.getStmt_dx(), direction::reverse); CompoundStmt* RCS = endBlock(direction::reverse); std::reverse(RCS->body_begin(), RCS->body_end()); Stmt* ReverseResult = unwrapIfSingleStmt(RCS); @@ -3824,9 +3836,14 @@ Expr* getArraySizeExpr(const ArrayType* AT, ASTContext& context, Expr* clonedNewE = utils::BuildCXXNewExpr( m_Sema, CNE->getAllocatedType(), clonedArraySizeE, initializerDiff.getExpr(), CNE->getAllocatedTypeSourceInfo()); + Expr* diffInit = initializerDiff.getExpr_dx(); + if (!diffInit) { + // we should initialize it implicitly using ParenListExpr. + diffInit = m_Sema.ActOnParenListExpr(noLoc, noLoc, {}).get(); + } Expr* derivedNewE = utils::BuildCXXNewExpr( - m_Sema, CNE->getAllocatedType(), derivedArraySizeE, - initializerDiff.getExpr_dx(), CNE->getAllocatedTypeSourceInfo()); + m_Sema, CNE->getAllocatedType(), derivedArraySizeE, diffInit, + CNE->getAllocatedTypeSourceInfo()); return {clonedNewE, derivedNewE}; } diff --git a/test/ForwardMode/Pointer.C b/test/ForwardMode/Pointer.C index 5190f5cc5..bb1edfdde 100644 --- a/test/ForwardMode/Pointer.C +++ b/test/ForwardMode/Pointer.C @@ -161,7 +161,7 @@ double fn7(double i) { // CHECK-NEXT: t->i = i; // CHECK-NEXT: double _d_res = *_d_p + _d_t->i; // CHECK-NEXT: double res = *p + t->i; -// CHECK-NEXT: unsigned long _t2 = sizeof(double); +// CHECK-NEXT: unsigned {{(int|long)}} _t2 = sizeof(double); // CHECK-NEXT: {{(clad::)?}}ValueAndPushforward _t3 = clad::custom_derivatives::realloc_pushforward(p, 2 * _t2, _d_p, 0 * _t2 + 2 * sizeof(double)); // CHECK-NEXT: _d_p = (double *)_t3.pushforward; // CHECK-NEXT: p = (double *)_t3.value; diff --git a/test/Gradient/Pointers.C b/test/Gradient/Pointers.C index c216aba6e..fe3f0e58a 100644 --- a/test/Gradient/Pointers.C +++ b/test/Gradient/Pointers.C @@ -363,7 +363,7 @@ double newAndDeletePointer(double i, double j) { // CHECK-NEXT: double *p = new double(i); // CHECK-NEXT: _d_q = new double(* _d_j); // CHECK-NEXT: double *q = new double(j); -// CHECK-NEXT: _d_r = new double [2]; +// CHECK-NEXT: _d_r = new double [2](/*implicit*/(double{{[ ]?}}[2])0); // CHECK-NEXT: double *r = new double [2]; // CHECK-NEXT: _t0 = r[0]; // CHECK-NEXT: r[0] = i + j; @@ -418,7 +418,7 @@ double structPointer (double x) { // CHECK: void structPointer_grad(double x, clad::array_ref _d_x) { // CHECK-NEXT: T *_d_t = 0; // CHECK-NEXT: double _d_res = 0; -// CHECK-NEXT: _d_t = new T; +// CHECK-NEXT: _d_t = new T(); // CHECK-NEXT: T *t = new T({x, /*implicit*/(int)0}); // CHECK-NEXT: double res = t->x; // CHECK-NEXT: goto _label0; @@ -432,6 +432,7 @@ double structPointer (double x) { double cStyleMemoryAlloc(double x, size_t n) { T* t = (T*)malloc(n * sizeof(T)); + memset(t, 0, n * sizeof(T)); t->x = x; double* p = (double*)calloc(1, sizeof(double)); *p = x; @@ -457,6 +458,8 @@ double cStyleMemoryAlloc(double x, size_t n) { // CHECK-NEXT: double _t5; // CHECK-NEXT: _d_t = (T *)malloc(n * sizeof(T)); // CHECK-NEXT: T *t = (T *)malloc(n * sizeof(T)); +// CHECK-NEXT: memset(_d_t, 0, n * sizeof(T)); +// CHECK-NEXT: memset(t, 0, n * sizeof(T)); // CHECK-NEXT: _t0 = t->x; // CHECK-NEXT: t->x = x; // CHECK-NEXT: _d_p = (double *)calloc(1, sizeof(double));