vgvassilev / clad

clad -- automatic differentiation for C/C++
GNU Lesser General Public License v3.0
266 stars 113 forks source link

`clad::hessian` doesn't like pointer dereferencing #961

Open guitargeek opened 1 week ago

guitargeek commented 1 week ago

Arrays are used everywhere in the code generated by RooFit, so this needs to be supported also in Hessian mode.

Reporducer:

#include <Math/CladDerivator.h>

double roo_func_wrapper_0(double* params) {
   double arr[] = {3.0};
   return params[0] + *arr;
}

#pragma clad ON
clad::gradient(roo_func_wrapper_0, "params"); // gradient works fine!
clad::hessian(roo_func_wrapper_0, "params[0:1]");
// It also doesn't work with:
//clad::hessian<clad::opts::diagonal_only>(roo_func_wrapper_0, "params[0]");
#pragma clad OFF

void macro()
{
   std::vector<double> parametersVec = {1.0};

   roo_func_wrapper_0(parametersVec.data());
}

Output:

warning: function 'cling_runtime_internal_throwIfInvalidPointer' was not differentiated because clad failed to differentiate it and no suitable overload was found in namespace 'custom_derivatives', and function may not be eligible for numerical differentiation.
warning: indirection of non-volatile null pointer will be deleted, not trap [-Wnull-dereference]
note: consider using __builtin_trap() or qualifying pointer with 'volatile'
warning: function 'cling_runtime_internal_throwIfInvalidPointer' was not differentiated because clad failed to differentiate it and no suitable overload was found in namespace 'custom_derivatives', and function may not be eligible for numerical differentiation.
warning: indirection of non-volatile null pointer will be deleted, not trap [-Wnull-dereference]
note: consider using __builtin_trap() or qualifying pointer with 'volatile'
vaithak commented 1 week ago

I am unable to reproduce this with Clang. This works correctly with Clang as intended; is this an issue with Cling? One thing that seems wrong here is that we specify the index from start to end (both included) for hessian. So, this params[0:1] means there are two elements inside params by which we compute the hessian against, which is wrong (as params only has 1 element). @guitargeek can you verify once if params[0] only fixes it? Essentially making sure that the last index is included.

guitargeek commented 1 week ago

Using params[0] didn't fix it. Did you try to run the reproducer with ROOT?

vaithak commented 1 week ago

I am trying that next. I will let you know if I find something.

vgvassilev commented 1 week ago

I am thinking that we should put somehow cling in the pull request pre-merge checks…

vgvassilev commented 1 week ago

@guitargeek, which interface of cling we use to run the code through? It seems we have some noise from the null pointer checks. We can disable that with a pragma in ROOT IIRC or using an interface which does not enable that feature…

guitargeek commented 1 week ago

I was running the code as a ROOT macro, i.e. putting the reproducer in macro.C and then running it like root macro.C.

You get the same error in any ROOT setup, for example also on lxplus with 6.32.02:

source /cvmfs/sft.cern.ch/lcg/app/releases/ROOT/6.32.02/x86_64-almalinux9.4-gcc114-opt/bin/thisroot.sh
root macro.C
vgvassilev commented 6 days ago

I am wondering how is this different than the regular calls where we avoid cling_runtime_internal_throwIfInvalidPointer. I see that TFormula goes via gInterpreter->Declare and your macro must be going via gInterpreter->ProcessLine or something like that...

vaithak commented 6 days ago

So, I tested this out with a simple example. This seems to be unrelated to Hessian, but fails with normal forward mode too. I printed the source fn AST, turns out this function is added in the source AST itself.

Clad is unable to differentiate this function and hence the warning:

warning: function 'cling_runtime_internal_throwIfInvalidPointer' was not differentiated because clad failed to differentiate it and no suitable overload was found in namespace 'custom_derivatives', and function may not be eligible for numerical differentiation.
vgvassilev commented 6 days ago

Maybe we should add a custom propagator for this function in ROOT. Can you take a look?

vgvassilev commented 1 day ago

I am wondering how is this different than the regular calls where we avoid cling_runtime_internal_throwIfInvalidPointer. I see that TFormula goes via gInterpreter->Declare and your macro must be going via gInterpreter->ProcessLine or something like that...

The way that R__CLING_PTRCHECK which controls whether ROOT should generate cling_runtime_internal_throwIfInvalidPointer is suboptimal to say the least. Apparently there is not an easy way to disable this. It was implemented via annotations and there is no pragma on/off supported...

vgvassilev commented 1 day ago

I just found an odd way to test it:

root.exe -l -b
root [0] .rawInput
Using raw input
root [1] #include <Math/CladDerivator.h>
root [2] double roo_func_wrapper_0(double* params) {
root (cont'ed, cancel with .@) [3]   double arr[] = {3.0};
root (cont'ed, cancel with .@) [4]   return params[0] + *arr;
root (cont'ed, cancel with .@) [5]}
root [6] #pragma clad ON
root [7] auto grad = clad::gradient(roo_func_wrapper_0, "params"); // gradient works fine!
root [8] auto hess = clad::hessian(roo_func_wrapper_0, "params[0:1]");
root [9] #pragma clad OFF
root [10] .rawInput
Not using raw input
root [11] std::vector<double> parametersVec = {1.0};
root [12] roo_func_wrapper_0(parametersVec.data());
root [13] roo_func_wrapper_0(parametersVec.data())
(double) 4.0000000
root [14] 
vgvassilev commented 21 hours ago
diff --git a/interpreter/cling/lib/Interpreter/ClingPragmas.cpp b/interpreter/cling/lib/Interpreter/ClingPragmas.cpp
index f9179e3d61..a5d02bc1b5 100644
--- a/interpreter/cling/lib/Interpreter/ClingPragmas.cpp
+++ b/interpreter/cling/lib/Interpreter/ClingPragmas.cpp
@@ -60,6 +60,7 @@ namespace {
       kArgumentsAreLiterals,

       kOptimize,
+      kPtrCheck,
       kInvalidCommand,
     };

@@ -120,6 +121,8 @@ namespace {
         return kAddInclude;
       else if (CommandStr == "optimize")
         return kOptimize;
+      else if (CommandStr == "ptrcheck")
+        return kPtrCheck;
       return kInvalidCommand;
     }

@@ -209,6 +212,18 @@ namespace {
         CO.OptLevel = OptLevel;
   }

+  void PtrCheckCommand(const char* Str) {
+    char* ConvEnd = nullptr;
+    // FIXME: Check better for On/Off/Default and invalid input.
+    int PtrCheckVal = std::strtol(Str, &ConvEnd, 10 /*base*/);
+    auto T = const_cast<Transaction*>(m_Interp.getCurrentTransaction());
+    assert(T && "Parsing code without transaction!");
+    // The topmost Transaction drives the jitting.
+    T = T->getTopmostParent();
+    CompilationOptions& CO = T->getCompilationOpts();
+    CO.CheckPointerValidity = PtrCheckVal;
+  }
+
   public:
     ClingPragmaHandler(Interpreter& interp):
       PragmaHandler("cling"), m_Interp(interp) {}
@@ -250,6 +265,8 @@ namespace {
         return LoadCommand(PP, Tok, std::move(Literal));
         case kOptimize:
           return OptimizeCommand(Literal.c_str());
+        case kPtrCheck:
+          return PtrCheckCommand(Literal.c_str());

         default:
           do {

This patch allow us to do something like that in ROOT:

root.exe -l -b 
root [0] gInterpreter->Declare("#include <Math/CladDerivator.h> \n #pragma cling ptrcheck 0 \n double roo_func_wrapper_0(double* params) { double arr[] = {3.0}; return params[0] + *arr; } \n auto h = clad::hessian(roo_func_wrapper_0, \"params[0:1]\");")
(bool) true
root [1] .q