Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C++ interface templates appear to be broken #1904

Closed
GregTheMadMonk opened this issue May 23, 2024 · 9 comments
Closed

C++ interface templates appear to be broken #1904

GregTheMadMonk opened this issue May 23, 2024 · 9 comments

Comments

@GregTheMadMonk
Copy link
Contributor

GregTheMadMonk commented May 23, 2024

Hello! Playing with C++ interface trying to get it to work and this simple program:

#include <vector>

#include <enzyme/enzyme>

using Flt = double;
using Vec = std::vector<Flt>;

Flt sum(const Vec& x) {
    Flt ret = 0.0;
    for (const auto xi : x) ret += xi;
    return ret;
} // <-- sum

Vec dsum(const Vec& x) {
    Vec dx(x.size());
    return enzyme::get<0>(
        enzyme::autodiff<enzyme::Forward>(
            sum,
            enzyme::DuplicatedNoNeed<const Vec&>{ x, dx }
        )
    );
} // <-- dsum

int main() {
    return 0;
}

doesn't compile with the following errors:

In file included from /media/ssd/projects/cpp/enzyme/enzyme-tests/cpp-interface-test/main.cc:3:
In file included from /enzymeroot/enzyme/enzyme:3:
/enzymeroot/enzyme/utils:68:7: error: multiple overloads of 'DuplicatedNoNeed' instantiate to the same signature 'void (const std::vector<double> &, const std::vector<double> &)'
   68 |       DuplicatedNoNeed(T&& v, T&& s) : value(v), shadow(s) {};
      |       ^
/media/ssd/projects/cpp/enzyme/enzyme-tests/cpp-interface-test/main.cc:19:13: note: in instantiation of template class 'enzyme::DuplicatedNoNeed<const std::vector<double> &>' requested here
   19 |             enzyme::DuplicatedNoNeed<const Vec&>{ x, dx }
      |             ^
/enzymeroot/enzyme/utils:67:7: note: previous declaration is here
   67 |       DuplicatedNoNeed(const T& v, const T& s) : value(v), shadow(s) {};
      |       ^
In file included from /media/ssd/projects/cpp/enzyme/enzyme-tests/cpp-interface-test/main.cc:1:
In file included from /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/14.1.1/../../../../include/c++/14.1.1/vector:86:
In file included from /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/14.1.1/../../../../include/c++/14.1.1/bits/memory_resource.h:41:
In file included from /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/14.1.1/../../../../include/c++/14.1.1/bits/uses_allocator_args.h:39:
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/14.1.1/../../../../include/c++/14.1.1/tuple:2672:45: error: implicit instantiation of undefined template 'std::tuple_size<enzyme::tuple<int *&&, int &&, const std::vector<double> &&, const std::vector<double> &&>>'
 2672 |     : __make_tuple_impl<0, tuple<>, _Tuple, tuple_size<_Tuple>::value>
      |                                             ^
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/14.1.1/../../../../include/c++/14.1.1/tuple:2678:14: note: in instantiation of template class 'std::__do_make_tuple<enzyme::tuple<int *&&, int &&, const std::vector<double> &&, const std::vector<double> &&>>' requested here
 2678 |     : public __do_make_tuple<__remove_cvref_t<_Tuple>>
      |              ^
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/14.1.1/../../../../include/c++/14.1.1/tuple:2709:19: note: in instantiation of template class 'std::__make_tuple<enzyme::tuple<int *&&, int &&, const std::vector<double> &&, const std::vector<double> &&>>' requested here
 2709 |         <typename __make_tuple<_Tpls>::__type...>::__type __type;
      |                   ^
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/14.1.1/../../../../include/c++/14.1.1/tuple:2776:17: note: in instantiation of template class 'std::__tuple_cat_result<enzyme::tuple<int *&&, int &&, const std::vector<double> &&, const std::vector<double> &&>>' requested here
 2776 |     -> typename __tuple_cat_result<_Tpls...>::__type
      |                 ^
/enzymeroot/enzyme/tuple:127:10: note: while substituting deduced template arguments into function template 'tuple_cat' [with _Tpls = <tuple<int *&&, int &&, const vector<double, allocator<double>> &&, const vector<double, allocator<double>> &&>>]
  127 |   return tuple_cat(concat_with_fwd_tuple< iseq<FWD_TUPLE>, iseq<first> >::f(impl::forward<FWD_TUPLE>(fwd), impl::forward<first>(t)), impl::forward<rest>(ts)...);
      |          ^
/enzymeroot/enzyme/tuple:135:16: note: in instantiation of function template specialization 'enzyme::impl::tuple_cat<enzyme::tuple<int *>, enzyme::tuple<int, const std::vector<double> &, const std::vector<double> &>>' requested here
  135 |   return impl::tuple_cat(impl::forward<Tuples>(tuples)...);
      |                ^
/enzymeroot/enzyme/utils:426:120: note: in instantiation of function template specialization 'enzyme::tuple_cat<enzyme::tuple<int *>, enzyme::tuple<int, const std::vector<double> &, const std::vector<double> &>>' requested here
  426 |         return autodiff_impl<return_type, DiffMode, function, functy, RetActivity>(impl::forward<function>(f), enzyme::tuple_cat(enzyme::tuple{detail::ret_used<DiffMode, RetActivity>::value}, expand_args(args)...));
      |                                                                                                                        ^
/media/ssd/projects/cpp/enzyme/enzyme-tests/cpp-interface-test/main.cc:17:17: note: in instantiation of function template specialization 'enzyme::autodiff<enzyme::ForwardMode, double (&)(const std::vector<double> &), enzyme::DuplicatedNoNeed<const std::vector<double> &>>' requested here
   17 |         enzyme::autodiff<enzyme::Forward>(
      |                 ^
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/14.1.1/../../../../include/c++/14.1.1/bits/utility.h:49:12: note: template is declared here
   49 |     struct tuple_size;
      |            ^
In file included from /media/ssd/projects/cpp/enzyme/enzyme-tests/cpp-interface-test/main.cc:3:
In file included from /enzymeroot/enzyme/enzyme:3:
/enzymeroot/enzyme/utils:407:82: error: static_cast from 'double (*)(const std::vector<double> &)' to 'double (*)(std::vector<double>)' is not allowed
  407 |       return detail::autodiff_apply<DiffMode>::template impl<return_type>((void*)static_cast<functy*>(f), detail::ret_global<RetActivity>::value, impl::forward<Tuple>(arg_tup), std::make_index_sequence<enzyme::tuple_size_v<Tuple>>{});
      |                                                                                  ^~~~~~~~~~~~~~~~~~~~~~~
/enzymeroot/enzyme/utils:426:16: note: in instantiation of function template specialization 'enzyme::autodiff_impl<enzyme::tuple<double>, enzyme::ForwardMode, double (&)(const std::vector<double> &), double (std::vector<double>), enzyme::DuplicatedNoNeed<double>, int *, int, std::vector<double>, std::vector<double>, 0>' requested here
  426 |         return autodiff_impl<return_type, DiffMode, function, functy, RetActivity>(impl::forward<function>(f), enzyme::tuple_cat(enzyme::tuple{detail::ret_used<DiffMode, RetActivity>::value}, expand_args(args)...));
      |                ^
/media/ssd/projects/cpp/enzyme/enzyme-tests/cpp-interface-test/main.cc:17:17: note: in instantiation of function template specialization 'enzyme::autodiff<enzyme::ForwardMode, double (&)(const std::vector<double> &), enzyme::DuplicatedNoNeed<const std::vector<double> &>>' requested here
   17 |         enzyme::autodiff<enzyme::Forward>(
      |                 ^
3 errors generated.

It seems that the interface

  1. Does not quite like the use of references :)
  2. Uses some std:: stuff with an internal enzyme::tuple

Unless I terribly misunderstand something, this is an error.

If it is, I think I'm going to try and fix it

edit: btw, what version of C++ should I use if editing the headers? C++17, or do you target lower versions too?

@wsmoses
Copy link
Member

wsmoses commented May 24, 2024

Yeah it is known that references aren't handled properly in the new interface, help definitely welcome!

And yeah tentaitlvely c++17, though obviously earlier support would be nice if possible

@jandrej
Copy link
Contributor

jandrej commented Jun 2, 2024

Adding a MFE which used to work in the original PR for the C++ interface https://fwd.gymni.ch/nsKPWF

@GregTheMadMonk
Copy link
Contributor Author

@jandrej with #1914 your code compiles with the following minor changes:

62,64c62,63
<    return std::tuple<enzyme::Duplicated<decltype(std::get<Is>(args))>...>{
<       { std::get<Is>(args), std::get<Is>(shadow_args) }...
<    };
---
>    return std::tuple_cat(std::make_tuple(
>                             enzyme::Duplicated<std::remove_cv_t<std::remove_reference_t<decltype(std::get<Is>(args))>>*> {&std::get<Is>(args), &std::get<Is>(shadow_args)})...);
87c86
<                 enzyme::autodiff<enzyme::Forward>
---
>                 enzyme::autodiff<enzyme::Forward, enzyme::DuplicatedNoNeed<kf_return_t>>
106,110c105
<     std::get<0>(kernel_args) = 3;
<     std::get<0>(kernel_shadow_args) = 1;
<
<     const auto res = fwddiff_apply_enzyme(func, kernel_args, kernel_shadow_args);
<     std::cout << res << " == 6\n";
---
>     fwddiff_apply_enzyme(func, kernel_args, kernel_shadow_args);

(I've replaced std::make_tuple(std::tuple_cat(... with a std::tuple constructor to make it compile with C++23, which I use. Note that the enzyme::Duplicated argument has also changed from the pointer to a reference. Waiting for a response on my PR to know if I understood Duplicated/DuplicatedNoNeed right).

It still doesn't work as intended (prints 0 == 6), since, as I've discovered, the C++ interface also doesn't process function pointers correctly. For example, this code

#include <enzyme/enzyme>
#include <iostream>

double f(const double& x) { return x * x; }

int main() {
    double x = 3, dx = 0;
    enzyme::autodiff<enzyme::Reverse>(
        f, enzyme::Duplicated<const double&>{ x, dx }
    );
    std::cout << x << ' ' << dx << '\n';
    return 0;
}

will compile and work meanwhile this:

#include <enzyme/enzyme>
#include <iostream>

double f(const double& x) { return x * x; }

int main() {
    double x = 3, dx = 0;
    enzyme::autodiff<enzyme::Reverse>(
        &f, enzyme::Duplicated<const double&>{ x, dx }
    );
    std::cout << x << ' ' << dx << '\n';
    return 0;
}

will not:

error: Enzyme: No create nofree of unknown value
ptr %2
 at context:   %4 = tail call noundef double %3(ptr noundef nonnull align 8 dereferenceable(8) %0) #7
1 error generated.
make[2]: *** [CMakeFiles/test.dir/build.make:76: CMakeFiles/test.dir/main.cc.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:910: CMakeFiles/test.dir/all] Error 2
make: *** [Makefile:136: all] Error 2

(nevermind the line numbers, I'm testing everything in a single main.cc with a few #if 0 ... #endif's)

@wsmoses
Copy link
Member

wsmoses commented Jun 2, 2024

@GregTheMadMonk the issue is that we need to ensure that all the reference passes do not end up creating a copy and thus the autodiff/fwddiff call gets the pointer to the actual reference provided.

Re function pointers, they are supported, but when passed directly. (like the top)

@GregTheMadMonk
Copy link
Contributor Author

@wsmoses Sorry, I'm not sure I quite follow what you're saying... Do you mean that we need to ensure that the pointer __enzyme_autodiff/__enzyme_fwddiff end up getting point to the same variables the user has provided when they have called the code? In that case there indeed will be no copying of data, only of references and pointer to it (but I'll re-check it).

Then there is also the issue that I've written about in the PR comments - but I see you've already read and replied to it :)

And I also realize (now) why function pointers can't really be processed (since a function pointer could point to any function really and there is no guarantee it was processed by Enzyme). __enzyme_autodiff can process some though (e.g. when passing a compile-time known function pointer via &f or +lambda). Should this be preserved, or outright prohibited (ideally via a static_assert with a message explaining why function pointers are not to be used in this context and what could be used instead)?

It seems that the best self-critique arrives after the code has been submitted...

It's getting quite late, I won't be replying for a while

@wsmoses
Copy link
Member

wsmoses commented Jun 2, 2024

So we can find out what's happening by looking at the generated LLVM. If there's a copy somewhere we'll see a different allocation passed to the underlying __enzyme_autodiff (which is my guess why it could get a different answer than expected).

It would be nice to do a check at the outermost c++ api level (though we have some internal shenanigans to handle a slightly wider scope).

@jandrej
Copy link
Contributor

jandrej commented Jun 3, 2024

@GregTheMadMonk did you check the output values? I only get zeros from the fwddiff in your branch.
edit:
Just understood your comment now. I hope you find the problem, I'm happy to test.

@wsmoses
Copy link
Member

wsmoses commented Jul 22, 2024

Seemingly resolved.

@wsmoses wsmoses closed this as completed Jul 22, 2024
@GregTheMadMonk
Copy link
Contributor Author

Thank you! Sorry for being very inconsistent with GH activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants