NilFoundation / zkLLVM

Zero-Knowledge Proof Systems Circuit Compiler
https://docs.nil.foundation/zkllvm
274 stars 46 forks source link

[Possible bug]: Constant element of constant array should interpreted as constant. #535

Open ETatuzova opened 5 months ago

ETatuzova commented 5 months ago

i-st element of constexpr array for constant i is definitely constant. But we cannot use it as for-loop number of iterations.

#include <nil/crypto3/algebra/curves/pallas.hpp>

constexpr std::array<std::size_t, 5> sizes = {1,2,3,4,5};

using namespace nil::crypto3::algebra::curves;

[[circuit]] typename pallas::base_field_type::value_type
    field_arithmetic_example(typename pallas::base_field_type::value_type a,
                             typename pallas::base_field_type::value_type b) {

    for( std::size_t i = 0; i < sizes[1]; i++ ){
        a = a + b;
    }
    return a;
}
akokoshn commented 5 months ago

I guess it's not a bug. I terms of LLVM sizes[1] is load i32, ptr %0, align 4 where %0 is pointer to allocated memory. Compiler can't to calculate result of the load instruction. @makxenov , please correct me if i wrong.

aleasims commented 5 months ago

operator[] have to be constexpr here, so sizes[1] should have been resolved to constant. No loads must be in output IR. It seems to me like that, at least.

ETatuzova commented 5 months ago

I used this construction to emulate vector of vectors logic.

constexpr std::size_t sizes_size = 3;
constexpr std::array<int, sizes_size> sizes = {1,2,3};
constexpr std::size_t full_size = 6;

So, we could use this:

std::array<int, full_size> arr = {1, 2, 2, 3, 3, 3};
// In fact it is vector of vectors[[1], [2, 2], [3,3,3]]
....
std::size_t cur = 0;
for(std::size_t i = 0; i < sizes_size; i++){
    for(std::size_t j = 0; j < sizes[i]; j++, cur++){
       //Do something with arr[cur] that is in fact vector_of_vectors[i][j]
    }
}

But now we can't, because for-loop bound should be constant expression. What is the best way to implement it now? I enrolled most of loops in generated code manually. But it's non-readable.

akokoshn commented 5 months ago

As far i see std::array<T, N>::operator[] is not constexpr: https://en.cppreference.com/w/cpp/container/array/operator_at In the last example we'll have problem any way with unrolling internal loop (https://github.com/NilFoundation/zkLLVM/issues/522).

@ETatuzova , could you please share full circuit code example, if it possible, i'll rty to think how to modify it (some thing like manual loop fuse)

aleasims commented 5 months ago

https://en.cppreference.com/w/cpp/container/array/operator_at

It says "(constexpr since C++17)" on your link, isn't it?

akokoshn commented 5 months ago

Hmm..., you are right. If write:

constexpr std::size_t size = sizes[1];
for( std::size_t i = 0; i < size; i++ ){
    a = a + b;
}

unrolled, but

for( std::size_t i = 0; i < sizes[1]; i++ ){
    a = a + b;
}

not.

akokoshn commented 5 months ago

Investigation info: on calculation loop range: ScalarEvolution::SimplifyICmpOperands(...) (file ~/zkLLVM/libs/circifier/llvm/lib/Analysis/ScalarEvolution.cpp). RHS is constant for 1th case, and result of 'load' for second

makxenov commented 5 months ago

constexpr function is not guaranteed to be evaluated in compile time. I think we could try to force it for our target, in similar way as we did for loops.