More advanced activity analysis and many other bugfixes #33
Conversation
There was a problem hiding this comment.
- you committed output files (e.g "op", etc.) which should be removed.
- restore the functional_tests_c directory and the makefile target. It doesn't need to be in the CI, but I'd like to continue using it.
- your changes do not have comments --- if you could make an effort to commenting your code, it would be appreciated.
Cheers,
Tim
| @@ -48,4 +48,3 @@ message("found llvm version " ${LLVM_VERSION_MAJOR}) | |||
|
|
|||
| add_subdirectory(Enzyme) | |||
| add_subdirectory(test) | |||
| add_subdirectory(functional_tests_c) | |||
There was a problem hiding this comment.
keep this subdirectory and the cmake target. CI doesn't need to run it though.
|
|
||
| assert(lc.latchMerge); | ||
| static std::map<std::tuple<GradientUtils*, BasicBlock*, BasicBlock*>, BasicBlock*> mp; |
There was a problem hiding this comment.
Why is a gradientutils pointer address being used as part of the key? That is not guaranteed to be unique is it?
| @@ -952,7 +959,12 @@ class GradientUtils { | |||
| if (size == nullptr) { | |||
| size = limits[i]; | |||
| } else { | |||
| size = allocationBuilder.CreateNUWMul(size, limits[i]); | |||
| static std::map<std::pair<Value*, BasicBlock*>, Value*> sizeCache; | |||
There was a problem hiding this comment.
Is using a static map with keys <Value*, BasicBlock*> okay? Are entries deleted from the sizecache in the destructor of gutils?
|
|
||
| constexpr size_t IN = 4, OUT = 4, NUM = 5; | ||
| __attribute__((noinline)) | ||
| static double matvec(const Matrix<double, IN, OUT>* __restrict W, const Matrix<double, IN, OUT>* __restrict M) { |
There was a problem hiding this comment.
Perhaps add an expect fail test for this where we use MatrixXf
There was a problem hiding this comment.
few more comments on the tests. In general, with the exception of the flag for globals, we should have the same set of commands on all testfiles. If a command fails for certain tests, just have it be expect fail. If we just omit tests then its too easy for us to forget that we have an outstanding bug when compiling with certain settings on certain tests.
| @@ -0,0 +1,113 @@ | |||
| // RUN: %clang -std=c11 -O0 %s -S -emit-llvm -o - | %opt - %loadEnzyme -enzyme -S | %lli - | |||
There was a problem hiding this comment.
How is this test different from insertsort_sum? It looks like a copy of an older version of that test before extraneous stuff was deleted from the main function)
Unfortunately digging deeper into the code, I believe a major rewrite to reverse pass loops is necessary for correctness -- this is the extent of the fixes before work on that rewrite began.