Skip to content

Improve performance by fusing kernels - #57

Open
au2001 wants to merge 11 commits into
1inch:masterfrom
codesensus-org:perf/fuse-kernels-and-align
Open

Improve performance by fusing kernels#57
au2001 wants to merge 11 commits into
1inch:masterfrom
codesensus-org:perf/fuse-kernels-and-align

Conversation

@au2001

@au2001 au2001 commented Jul 26, 2026

Copy link
Copy Markdown

Change Summary

What does this PR change?

It improves performance by 8-12% with these compounding changes:

  • aligning mp_number to 2x 16 bytes instead of 8x 4 bytes; and
  • merging kernels for iterating/transforming/scoring into a single one

(profanity_inverse is untouched in this PR, so there are still 2 launches total, down from 3-4)

Testing & Verification

How was this tested?

  • Unit tests
  • Integration tests
  • Manual testing (describe steps)
  • Verified on staging

I ran the --leading 0 scoring method on different GPUs with the base and head codebase.
For each resulting address, I ran an external verification script which found no mistake.
Here are the results for "70" variant of each NVIDIA generation:

GPU VRAM base head inverse-multiple gain
RTX 5070 12 GB 787 MH/s 876 MH/s -I 393216 +11.3%
RTX 4070 12 GB 659 MH/s 712 MH/s -I 393216 +8.0%
RTX 3070 8 GB 487 MH/s 536 MH/s -I 262144 +10.1%
RTX 2070 8 GB 328 MH/s 359 MH/s -I 16384 +9.4%
GTX 1070 8 GB 201 MH/s 225 MH/s -I 196608 +12.2%

Edit: Previous results used --benchmark, for which the compiler optimized the kernel by deleting the keccak + scoring. A patch has been added to prevent this from happening.

Notes:
During benchmarks, when reaching speeds of 2-3ms per round, the millisecond clock precision was insufficient.
I thus bumped it to microseconds for increased accuracy.

A bug also prevented from enabling profiling: clCreateCommandQueueWithProperties on OpenCL 2.0+ expects a zero terminated list of name/value pairs, not a bitfield like 1.2 – 0 just so happened to be interpreted as an empty list.

Risk Assessment

Risk Level:

  • Low - Minor changes, no operational impact
  • Medium - Moderate changes, limited impact, standard rollback available
  • High - Significant changes, potential operational impact, complex rollback

Risks & Impact

Fully backwards-compatible with no user-facing changes.
But changes how the kernels work in a non-trivial way.
Requires clearing the cache to see changes (and to rollback).

Disclaimer: this PR was assisted by Claude Opus 5 for code exploration & benchmark scripts.
However, all code being submitted has been hand-written – and I'm a human :)

@cursor

cursor Bot commented Jul 30, 2026

Copy link
Copy Markdown

Thanks for this — the fusion and the 16-byte alignment are both well-motivated, the commit history is easy to follow, and the two side fixes (queue properties, microsecond timing) are worth having on their own.

I checked that the refactor is behaviour-preserving: the doubles bit trick is equivalent for all 256 byte values, profanity_byte() matches the old uchar reinterpretation over random addresses, and the word-wise --exact compare matches the old byte-wise one over random masks built the way Mode::matching builds them. Dropping the pInverse address write is safe too, since profanity_inverse rewrites that whole range every round.

One blocker, then a few smaller asks.

Blocker: the inline helpers need to be static inline

OpenCL C follows C99, where inline without static or extern is an inline definition rather than an external one. If the compiler declines to inline a call, no body is emitted and the symbol is left undefined. That is what happens here with the clang front end that ROCm, PoCL, Intel NEO, Mesa Rusticl and Apple all use:

$ cat keccak.cl profanity.cl | clang -x cl -cl-std=CL1.2 -Xclang -finclude-default-header \
    -c -O2 -DPROFANITY_INVERSE_SIZE=255 -DPROFANITY_MAX_SCORE=40 -o /tmp/pr57.o -
$ nm -u /tmp/pr57.o | grep profanity
                 U profanity_iterate
                 U profanity_score_fn_mirror

Undefined symbols by optimisation level: -O0 → 10 (i.e. all ten helpers), -O1 → 1, -O2 → 2, -O3 → 4. master is clean at every level, so there is no optimisation setting where this is safe. NVIDIA's front end inlines unconditionally, which is why none of your five cards show it.

Adding static to all ten inline functions (profanity_iterate, profanity_byte, and the eight profanity_score_fn_*) removes every undefined symbol at every level, keeps the -cl-std=CL1.2 -Werror build clean, and costs about 7% of emitted .text.

Running tests/ under PoCL is a cheap way to confirm this, since tests/testutil.hpp builds the full keccak.cl + profanity.cl + harness.cl program.

The stale kernel cache is a hard failure, not just stale performance

getDeviceCacheFilename() only encodes inverse-size and device id, so anyone who has run profanity2 before will load a cached binary that has no profanity_iterate_score_* kernel. clCreateKernel then returns NULL and the program exits with failed to create kernel "profanity_iterate_score_leading" instead of running at the old speed. The PR description reads as if the only consequence were not seeing the speedup.

Could you add a README note like the one #56 added? Making the cache key a hash of the sources plus build options would fix this class of problem permanently, but that is a separate PR and not yours to carry.

Please validate on at least one non-NVIDIA device

All five data points are NVIDIA. Fusing raises the amount of live state in a single kernel, and because bContract is a runtime argument the second ethhash stays in the register budget even for plain address runs — occupancy effects there are vendor-specific. The README still carries an RX 480 row and four Apple Silicon rows, and profanity.cl itself warns that the RX 480 is sensitive to changes in this area. One AMD and one Apple Silicon run would be enough to be comfortable.

Smaller things

  • pInverse is read-only in the fused kernel now, so __global const mp_number * const may buy a little more on NVIDIA through the read-only path.
  • SpeedSample::sample still divides by delta with no zero guard, which yields inf in the displayed speed. Microseconds make that far less likely, but you are touching that line anyway.
  • The README "Current version" benchmark table was measured with the old four-launch --benchmark, which no longer means the same thing after the fusion — worth a note or a refresh.
  • Minor consistency point: the contract path reads bytes through a uchar* cast while everything else goes through profanity_byte().

Nothing else in the diff concerned me. The curve constants, the RLP prefixes, seed generation and the private-key derivation are all untouched, and the result buffer still carries only the address.

@galekseev

Copy link
Copy Markdown

Hi, @au2001 . Thanks for the PR.
The blocker for merging is that it breaks compatibility with non-NVIDIA devices.
It would be great if you could fix it.

@au2001

au2001 commented Jul 31, 2026

Copy link
Copy Markdown
Author

Hi @galekseev, sorry for missing that, should be fixed now

I refreshed the README benchmarks too, but wasn't able to get an RX 480 specifically, so I left it at the old value

@cursor

cursor Bot commented Aug 2, 2026

Copy link
Copy Markdown

All four items are closed — thanks for the quick turnaround. I re-verified the updated branch, this time by actually building and running it under PoCL rather than only reading codegen.

A correction on my side first. I claimed the bare inline would break ROCm, PoCL, Intel and Apple. I have now tested PoCL 5.0 and it builds and runs the pre-fix version fine — its pipeline links the bitcode and force-inlines everything into the kernels, so the undefined symbol never materialises. The object-level evidence was real, but "will break" was too strong; the accurate statement is that the code was relying on inliner heuristics rather than being well-formed C99. static inline is still the right fix, and the branch now reports zero undefined symbols at every optimisation level, against 10/1/2/4 at -O0/-O1/-O2/-O3 before.

What I verified on aad8e13

  • Kernel builds clean under -Werror with -cl-std=CL1.2, CL2.0 and CL3.0. tests/ passes under PoCL. No new host compiler warnings — the two that appear are identical to master.
  • End-to-end correctness against an independent pure-Python secp256k1 + Keccak-256 reference that shares no code with the kernel: 863 key/address pairs verified, i.e. the seed private key plus the printed offset re-derives exactly the printed address. That covers --leading, --matching, --exact, --zeros, --letters, --numbers, --range, --leading-range, --mirror, --leading-doubles and --zero-bytes, plus --contract combined with both --leading and --exact, and a chunked launch via -W 4096.
  • 32 score values checked against an independent reimplementation of each scoring function. All match, so the refactor into profanity_score_fn_* is behaviour-preserving on real data and not just by inspection.
  • 2.2M --exact hits (plain and contract) all satisfy their mask.
  • The contract chain verifies in full: private key to sender address, then keccak(0xd6 0x94 addr 0x80) to contract address.
  • The stale-cache case reproduces exactly as described. With a cache written by master, the new binary exits with failed to create kernel "profanity_iterate_score_leading", and --no-cache clears it. The README note now at the top is well placed.
  • --benchmark matches --leading 0 throughput under PoCL, so the keccak is definitely no longer being eliminated.
  • Throughput on CPU is at parity. Measured externally via --exact hit rate rather than the internal counter, three runs each: master 5.13 / 5.02 / 5.31 MH/s, this branch 5.14 / 5.03 / 5.08 MH/s. That is expected — the round trips and launch overhead the fusion removes are cheap on a CPU device.

Incidentally that last measurement justifies the microsecond commit on its own: master reports exactly 8.192 MH/s at -i 32 -I 256, which is 8192 × 1000, i.e. the millisecond timer quantising a sub-millisecond round up to 1 ms.

Two small things left, neither blocking

1. RDNA2 is the remaining gap. Compiling for AMD through LLVM, the fused kernel needs 203 VGPRs against 189 for profanity_iterate, which drops occupancy from 5 to 4 waves on gfx1030. On gfx803 occupancy is already 1 and stays 1, and on gfx1100 registers are unchanged at 192 with occupancy 8 — so your RX 480 and RX 7900 XTX runs both land on generations where nothing changes:

Generation VGPR master → PR Occupancy
gfx803 (RX 480) 192 → 202 1 → 1
gfx1030 (RDNA2, RX 6000) 189 → 203 5 → 4
gfx1100 (RDNA3, RX 7900 XTX) 192 → 192 8 → 8

If anyone has an RX 6000-series card, one run would close this. It is not a merge blocker — the worst case is a regression confined to a single generation, and it would show up in a measurement.

2. The RX 480 row is a pre-fusion number sitting in a table of post-fusion numbers. You said as much in the thread, but the README does not, and the row's memory speed was changed from 2000 to 4000 while the "Modified straps: YES" column was dropped, which makes it read as freshly measured. A footnote would do.

Also worth updating the PR description: it still carries only the NVIDIA table, and "Requires clearing the cache to see changes" understates the effect — without clearing the cache the program does not start at all.

Nothing else came up. The curve constants, RLP prefixes, seed generation and private-key derivation remain untouched, and the result buffer still carries only the address.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants