zkLLVM icon indicating copy to clipboard operation
zkLLVM copied to clipboard

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

Open ETatuzova opened this issue 1 year ago • 8 comments

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;
}

ETatuzova avatar Feb 17 '24 19:02 ETatuzova

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.

akokoshn avatar Feb 18 '24 14:02 akokoshn

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.

aleasims avatar Feb 18 '24 15:02 aleasims

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.

ETatuzova avatar Feb 18 '24 15:02 ETatuzova

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)

akokoshn avatar Feb 18 '24 16:02 akokoshn

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

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

aleasims avatar Feb 18 '24 18:02 aleasims

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 avatar Feb 18 '24 20:02 akokoshn

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

akokoshn avatar Feb 19 '24 07:02 akokoshn

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.

makxenov avatar Feb 19 '24 07:02 makxenov