mirror of
https://github.com/pezkuwichain/revive.git
synced 2026-10-21 19:26:18 +00:00
0273a6b73bcfdbaa1bfefb4421903a71fdfbac4c
2
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
|
32763fedf2 |
Fix Windows bytecode divergence during LLVM LSR (#570)
## Description Our differential bytecode comparison tests detected a bytecode mismatch between the Linux/macOS builds and the Windows build for a [contract](https://github.com/paritytech/resolc-compiler-tests/blob/4647018d64eab6ccff30d22c753f02b90e9fc7e3/fixtures/solidity/simple/internal_function_pointers/sort/bubble.sol) implementing recursive bubble sort, specifically with these opt settings: * Solc optimizer disabled * Newyork enabled * `-Oz` This PR applies an [LLVM patch](https://github.com/llvm/llvm-project/pull/190587) which fixes the divergence for contracts with loops that index array elements at constant offsets from a Solidity `uint8` loop variable, observed with the above opt settings. ## Cause The first divergence occurs in an LLVM Loop Strength Reduction (LSR) decision, on `bubbleSort`'s loop (`for (uint8 i = 0; i < ARRAY_SIZE - start - 1; i++)` with `array[i]`/`array[i+1]` accesses): ```diff - %lsr.iv = phi i32 [ 0, %for_cond.preheader153 ], [ %lsr.iv.next, %if_join64 ] ; Linux/macOS + %lsr.iv = phi i32 [ 1, %for_cond.preheader153 ], [ %lsr.iv.next, %if_join64 ] ; Windows + %3 = add i32 %lsr.iv, -1 ; Windows ... - %3 = add i32 %lsr.iv, 1 ; Linux/macOS ... %lsr.iv.next = add nuw nsw i32 %lsr.iv, 1 ; identical on both platforms: step is +1 either way ``` In LLVM's `{Start,+,Step}` recurrence notation (value starts at `Start`, changes by `Step` per loop iteration), the three constants in the diff represent: recurrence *start*, use-site *offset*, and recurrence *step*. Only the first two differ between the platforms: **Linux/macOS** (the `array[i]` calls take `%lsr.iv` directly; the `array[i+1]` calls take `%3`): | Line | Constant | Role | |---|---|---| | `%lsr.iv = phi i32 [ 0, … ]` | `0` | **Start**: the register's value on loop entry (`i`) (holds 0, 1, 2, …) | | `%3 = add i32 %lsr.iv, 1` | `+1` | **Use-site offset**: derives the *other* index (`i + 1`) from the register each iteration. (No register is dedicated to `%3`) | | `%lsr.iv.next = add … , 1` | `+1` | **Step**: how much the register advances each iteration | **Windows** (mirrored: the `array[i+1]` calls take `%lsr.iv` directly; the `array[i]` calls take `%3`): | Line | Constant | Role | |---|---|---| | `%lsr.iv = phi i32 [ 1, … ]` | `1` | **Start**: the register's value on loop entry (`i + 1`) (holds 1, 2, 3, …) | | `%3 = add i32 %lsr.iv, -1` | `-1` | **Use-site offset**: derives the *other* index (`i`) from the register each iteration. (No register is dedicated to `%3`) | | `%lsr.iv.next = add … , 1` | `+1` | **Step**: identical | The two solutions are mirror images with exactly equal cost, because `getSetupCost()` (the patched function) rated *any* constant induction variable (IV) start equally (a 0-start and a 1-start IV cost the same). LSR keeps the first solution it enumerates, and the enumeration order derives from walking SSA use-lists, whose in-memory order differed between our Linux/macOS and Windows builds of LLVM. Thus both platforms compute the indices `i` and `i + 1` each iteration (functionally identical), but the LSR decision of which of the two recurrences becomes the loop-carried value (the phi) differs, which affects register allocation. > [!NOTE] > The equal cost tie can only arise if LSR processes the loop at all, and LSR only forms uses for induction variables of a 32-bit or 64-bit integer in our case. ## Partial solution The [upstream fix](https://github.com/llvm/llvm-project/pull/190587) updates `getSetupCost()` to return `0` for constant IV starts whose materialization is `TCC_Free` (which immediate `0` on RISC-V is), which is now cheaper than non-zero starts, resulting in the same LSR decision (no tie to decide on) on Windows as well. As zero-start IVs are most common, this fix addresses the most common scenarios, but it is still only a partial fix as it specifically solves only the zero-start case. (I'm currently writing up another issue regarding this.) ## Verification I created a draft LLVM release that was built with the patch applied, then reran all workflows using the draft release, including temporarily testing more opt settings for the bytecode comparisons which ran successfully (no mismatches) with the following `retester` modes (see [this run](https://github.com/paritytech/revive/actions/runs/28970814016/job/85982029937)): * `NY M0 S+` * `NY M0 S-` * `NY M3 S+` * `NY M3 S-` * `NY Mz S+` * `NY Mz S-` * `Y M0 S+` * `Y M0 S-` * `Y M3 S+` * `Y M3 S-` * `Y Mz S+` * `Y Mz S-` All E2E tests also ran as it does today (allowing all Yul and newyork IR `retester` modes), completing successfully. |
||
|
|
066c6d26f0 |
Fix rate-limited cloning of LLVM submodule (#569)
## Description We started seeing rate-limit `429` errors when cloning LLVM (as seen in e.g. [this failed run](https://github.com/paritytech/revive/actions/runs/28924583524/job/85809154518) and [this failed run](https://github.com/paritytech/revive/actions/runs/28926259175/job/85821336465?pr=564)). `llvm/llvm-project` now seems to be heavily rate-limited by GitHub. (Authenticating the clone did not seem to have much effect.) Even though our resolc build steps only do a treeless clone, all build steps still did the same clone. ### Changes * I've instead introduced a `.llvm-version` file that the `get-llvm` action reads from, eliminating the network request all together. * `.llvm-version` is meant to be kept in sync with the LLVM submodule version we use. This is enforced by: * Introduced a `check-llvm-version.yml` workflow * Runs once to check that the recorded version in `.llvm-version` coincides with the version detected from the submodule. * The submodule version detection *does* perform a network request as before, but this is only run once now (when relevant files change), rather than for all our resolc build jobs. * Since the differential test jobs require checking out our revive-differential-tests (rdt) submodule, those checkout steps cloned not only rdt, but also LLVM. I've updated this to not clone LLVM at all. |