Keccak CPU backend #622
Keccak CPU backend #622
Conversation
yshekel
requested review from
ChickenLover,
emirsoyturk,
mickeyasa and
aviadingo
| #define SHA3_USE_KECCAK_FLAG (0x80000000) | ||
| #define SHA3_CW(x) ((x) & (~SHA3_USE_KECCAK_FLAG)) | ||
|
|
||
| #if defined(_MSC_VER) |
There was a problem hiding this comment.
might consider adding modifers in
icicle/icicle/include/icicle/utils/modifiers.h
|
|
||
| for (unsigned batch_idx = 0; batch_idx < config.batch; ++batch_idx) { | ||
| eIcicleError err = sha3_hash_buffer( | ||
| 8 * digest_size_in_bytes, m_is_keccak, input + batch_idx * single_input_size, single_input_size, |
There was a problem hiding this comment.
worth adding a comment that the 8 is for byte to bit conversion
| @@ -61,7 +61,7 @@ if (HASH) | |||
| target_include_directories(test_hash_api PRIVATE ${CMAKE_SOURCE_DIR}/include/) | |||
| target_link_libraries(test_hash_api PRIVATE GTest::gtest_main icicle_device icicle_hash) | |||
| gtest_discover_tests(test_hash_api) | |||
| if (POSEIDON) | |||
| if (POSEIDON AND (FIELD OR CURVE)) | |||
There was a problem hiding this comment.
Maybe we should Have a flag for Hashes. maye we can replace POSEIDON with HASH to compile and test all hashes includiong poseidon
There was a problem hiding this comment.
we have a flag for hashes but then fields can also support poseidon but they don't have to so some will have it and some don't.
I should maybe set the poseidon to OFF in that case but I am not sure it's a good idea.
Will leave it like that for now
yshekel
force-pushed
the
yshekel/keccak
branch
from
a621ef8
to
3dc380b
Compare
General fixes including updated credit
| static const uint64_t s_keccakf_rndc[24]; | ||
| static const unsigned s_keccakf_rotc[24]; | ||
| static const unsigned s_keccakf_piln[24]; | ||
| const bool m_is_keccak; |
There was a problem hiding this comment.
what is that memeber for?
There was a problem hiding this comment.
The implementation that Aviad used needs to know that since it's both for keccak and sha3.
Since the same object implements keccak and sha3, it is stored. maybe an enum is better but I don't think it will be ever extended. If we need we can make it enum later, it's really just an implementation detail here.
| @@ -26,3 +26,9 @@ | |||
| #define __host__ | |||
| #define __device__ | |||
| #endif | |||
There was a problem hiding this comment.
we are not compatible with windows anyway.
There was a problem hiding this comment.
I know but the source was like that. Do you prefer deleting it? no way we support windows?
| @@ -134,6 +247,7 @@ class HashSumBackend : public HashBackend | |||
|
|
|||
| TEST_F(HashApiTest, MerkleTree) | |||
| { | |||
| ICICLE_CHECK(icicle_set_device(s_reference_target)); // TODO CUDA too | |||
There was a problem hiding this comment.
what about the TODO
we have cuda
There was a problem hiding this comment.
there is no merkle tree for GPU yet so the TODO is for Roman I guess :)
| // TODO: add tests for all hashes | ||
| TEST_F(HashApiTest, sha3) | ||
| { | ||
| auto config = default_hash_config(); |
There was a problem hiding this comment.
why are we testing it only on CPU?
There was a problem hiding this comment.
we test all devices including CUDA in the loop below
Implement Keccak and SHA-3 for CPU backend, including tests.
cuda-backend-branch: yshekel/keccak_cuda