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.
This commit is contained in:
LJ authored and GitHub committed 2026-07-09 13:17:28 +02:00
1 parent 16c255bf18
commit 32763fedf2
4 files changed
+82 -4

No files matched your search

+1 -1
View File
@@ -25,7 +25,7 @@ on:
jobs:
check-llvm-version:
runs-on: ubuntu-latest
runs-on: ubuntu-24.04
steps:
- name: Checkout revive
uses: actions/checkout@v6
+4 -3
View File
@@ -7,13 +7,14 @@ Supported `polkadot-sdk` rev: `2604.2.0`
### Fixed
- Restored the top-level `revive_version` field in `--standard-json` output and the `resolc_version` field in `--combined-json` output, both of which stopped being populated in `v0.4.0`. [#557](https://github.com/paritytech/revive/pull/557)
- ICE triggered by `--disable-solc-optimizer --newyork where inlined loops could emit an unterminated basic block in the inlined entrypoint. [#561](https://github.com/paritytech/revive/pull/561)
- ICE triggered by `--disable-solc-optimizer --newyork` where inlined loops could emit an unterminated basic block in the inlined entrypoint. [#561](https://github.com/paritytech/revive/pull/561)
- `--newyork`: dead-store elimination could drop a store whose memory expansion an intervening `msize()` observes, so `msize()` read a stale (unexpanded) value. [#565](https://github.com/paritytech/revive/pull/565)
- `--newyork`: dead-store elimination could drop a store that a later overlapping unaligned `mload` reads, returning zeroed memory instead of the stored bytes.
- `--newyork`: an unaligned full-word `mload`/`mstore` overlapping a word kept native (little-endian) by the heap optimization returned byte-swapped bytes.
- `--newyork`: dead-store elimination could drop a store that a later overlapping unaligned `mload` reads, returning zeroed memory instead of the stored bytes. [#563](https://github.com/paritytech/revive/pull/563)
- `--newyork`: an unaligned full-word `mload`/`mstore` overlapping a word kept native (little-endian) by the heap optimization returned byte-swapped bytes. [#562](https://github.com/paritytech/revive/pull/562)
- `--newyork`: a `calldatacopy`/`codecopy`/`returndatacopy` to a dynamic destination that could overwrite the free-memory-pointer word truncated a subsequent `mload(0x40)` to `0`. [#564](https://github.com/paritytech/revive/pull/564)
- `--newyork`: a full-width value stored to the free-memory-pointer slot (e.g. a function parameter `>= 2^64`) was narrowed to 64 bits, inserting a checked truncation that consumed all gas. [#567](https://github.com/paritytech/revive/pull/567)
- `--newyork`: an unaligned `mstore` corrupting the free-memory-pointer word let a later store through the corrupted pointer silently succeed where EVM runs out of gas. [#566](https://github.com/paritytech/revive/pull/566)
- Fixed a bytecode divergence (yet functionally equivalent) between the resolc Linux/macOS builds and the Windows build for contracts with loops indexing array elements at constant offsets from a `uint8` loop variable (e.g. `array[i]`/`array[i + 1]`), compiled with `--newyork` at `-Oz` with the solc optimizer disabled. [#570](https://github.com/paritytech/revive/pull/570)
## v1.3.0
+25
View File
@@ -0,0 +1,25 @@
# Third-Party Source Patches
Patches in this directory are applied on top of the corresponding third-party source files (e.g. the LLVM submodule) at build time. No manual step should be required, the build tooling applies them automatically.
## Layout and naming
* One subdirectory per patched project.
* LLVM patches live in `patches/llvm/`.
* Cherry-picks of upstream commits or pull requests are named accordingly.
* Example: `llvm-pr-190587.patch`
* `190587` would be the upstream PR number
## Format and content
* The `.patch` files must be `git apply`-compatible unified diffs, with paths relative to the submodule root (e.g. `a/llvm/lib/...`).
* The `.patch` files are forced to LF line endings via `.gitattributes`.
## Behavior
* Patches are applied in lexicographic order.
* Already-applied patches are detected and skipped, so re-running the build is safe.
## Lifecycle
Vendor a patch when a needed fix is not yet in the pinned revision, and delete the patch file when the pinned revision is bumped to one that contains the fix.
+52
View File
@@ -0,0 +1,52 @@
diff --git a/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp b/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
index a4c1e08430..2876353ee5 100644
--- a/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
@@ -1373,23 +1373,31 @@ static bool isAMCompletelyFolded(const TargetTransformInfo &TTI,
bool HasBaseReg, int64_t Scale,
Instruction *Fixup = nullptr);
-static unsigned getSetupCost(const SCEV *Reg, unsigned Depth) {
- if (isa<SCEVUnknown>(Reg) || isa<SCEVConstant>(Reg))
+static unsigned getSetupCost(const SCEV *Reg, unsigned Depth,
+ const TargetTransformInfo &TTI) {
+ if (isa<SCEVUnknown>(Reg))
return 1;
+ if (const auto *C = dyn_cast<SCEVConstant>(Reg)) {
+ if (TTI.getIntImmCost(C->getAPInt(), C->getType(),
+ TargetTransformInfo::TCK_RecipThroughput) ==
+ TargetTransformInfo::TCC_Free)
+ return 0;
+ return 1;
+ }
if (Depth == 0)
return 0;
if (const auto *S = dyn_cast<SCEVAddRecExpr>(Reg))
- return getSetupCost(S->getStart(), Depth - 1);
+ return getSetupCost(S->getStart(), Depth - 1, TTI);
if (auto S = dyn_cast<SCEVIntegralCastExpr>(Reg))
- return getSetupCost(S->getOperand(), Depth - 1);
+ return getSetupCost(S->getOperand(), Depth - 1, TTI);
if (auto S = dyn_cast<SCEVNAryExpr>(Reg))
return std::accumulate(S->operands().begin(), S->operands().end(), 0,
[&](unsigned i, const SCEV *Reg) {
- return i + getSetupCost(Reg, Depth - 1);
+ return i + getSetupCost(Reg, Depth - 1, TTI);
});
if (auto S = dyn_cast<SCEVUDivExpr>(Reg))
- return getSetupCost(S->getLHS(), Depth - 1) +
- getSetupCost(S->getRHS(), Depth - 1);
+ return getSetupCost(S->getLHS(), Depth - 1, TTI) +
+ getSetupCost(S->getRHS(), Depth - 1, TTI);
return 0;
}
@@ -1459,7 +1467,7 @@ void Cost::RateRegister(const Formula &F, const SCEV *Reg,
// Rough heuristic; favor registers which don't require extra setup
// instructions in the preheader.
- C.SetupCost += getSetupCost(Reg, SetupCostDepthLimit);
+ C.SetupCost += getSetupCost(Reg, SetupCostDepthLimit, *TTI);
// Ensure we don't, even with the recusion limit, produce invalid costs.
C.SetupCost = std::min<unsigned>(C.SetupCost, 1 << 16);