fix(hpu): erc20 whitepaper bench

[pgardratzama]

Full story is:

• @agnesLeroy updated the whitepaper ERC20 implementation in bench to match fhevm description but trivial encryption is not supported by HPU so when running ERC20 bench on HPU it fails
• I tried to replace if_then_else(cond, then, trivial(0)) by a if_then_zero which uses HPU IOp IF_THEN_ZERO and discovered that performance was better than ERC_20 IOp which was very strange
• @Bapt-Roux discovered ERC_20 IOp was incorrectly linked to ERC_20_SIMD and was not used kogge-stone for Add/Sub which explained poor performance
• @Bapt-Roux updated HLAPI if_then_zero for CPU and I tried to fix bench so that it could stay the same for GPU
• performance of ERC20 whitepaper and HPU_optimized moved from 31/34 TPS to 41/42 TPS on a single FPGA

I have run the CPU version on our HPU machines but not the GPU version.
So we need to check that it still works on GPU.
We also need to discuss if adding if_then_zero in HLAPI is ok?

[tmontaigu]

tfhe-benchmark/benches/high_level_api/erc20.rs line 44 at r1 (raw file):

        #[cfg(not(feature = "gpu"))]
        {
            has_enough_funds.if_then_zero(amount)

I dont know if we should do this optimisation as stricly speaking the whitepaper uses a select

[pgardratzama]

tfhe-benchmark/benches/high_level_api/erc20.rs line 44 at r1 (raw file):

        #[cfg(not(feature = "gpu"))]
        {
            has_enough_funds.if_then_zero(amount)

I dont know if we should do this optimisation as stricly speaking the whitepaper uses a select

I think it is a select, just a select between amount or 0, which is exactly what the whitepaper is describing, no?

[tmontaigu]

tfhe-benchmark/benches/high_level_api/erc20.rs line 44 at r1 (raw file):

Previously, pgardratzama (Pierre Gardrat) wrote…

I think it is a select, just a select between amount or 0, which is exactly what the whitepaper is describing, no?

Semantically yes, but in practice, the whitepaper cast 0 to a trivial then does select, and the way select is implemented (I believe all backends) will not check and see that one of the 'selectable' value is not an actual encryption and will still do a full select, here if_then_zero will do less work (half latency, 1/3 in total PBS)

So it depends on whether we want the benches here to reflect what the contract should be getting as perf on the blockchain, or if its another variant that is better like the others (no_cmux, overflow ,etc)

[pgardratzama]

Well @tmontaigu I have no idea what rules we want to follow here but my impression is that we wanted to do things faster.
Doing if_then_zero is faster than trivial_encrypt(0)+if_then_else and trivial_encrypt does panic when using HPU.

So either we go back to "pure" implementation of whitepaper ERC20 and I remove whitepaper ERC20 from HPU bench.
Or we implement whitepaper ERC20 in a smarter way...
Either way if fine by me just let me know what I need to do.

[IceTDrinker]

could use tests (even if it uses existing primitives)

[pgardratzama]

a test has been added for CPU & HPU... and it works both on CPU & FPGA!!!

[pgardratzama]

@tmontaigu summary of short discussion I had with @IceTDrinker:

• we should add support of trivial_encrypt in HPU, it could be useful at some point, let's add a backlog issue to track that
• we can for now replace the cfg(gpu/not_gpu) but cfg(not_hpu/hpu) so that we keep same whitepaper implementation on CPU & GPU and as soon as we have a trivial_encrypt support on HPU we can use it to have all the same code in ERC20 whitepaper

Are you ok with that?

[tmontaigu]

Yes seems good

[tmontaigu]

@tmontaigu reviewed 8 files and all commit messages, and resolved 2 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @soonum).

[soonum]

Good to me for benchmarks changes. 👍