fix(deterministic_id): include all code-sensitive config flags - #6871
fix(deterministic_id): include all code-sensitive config flags#6871marxin wants to merge 1 commit into
Conversation
We must ensure all the config values that influence the code generation are reflected in the compiled artifact location. Fixes: #6869
| String::from("singlepass-elf") | ||
| } else { | ||
| String::from("singlepass") | ||
| components.push("exp_art"); |
There was a problem hiding this comment.
What does exp_art mean?
Ah... experimental_artifact.
Why name it that way though?
That will just cause recompilation again later.
(The compilation cache uses the engine deterministic id!).
Can we pick a new name for it and stick to that?
There was a problem hiding this comment.
Actually, skeptical if this should go into the deterministic engine id at all ... the artifact format.
It sort of mixes up concerns.
There was a problem hiding this comment.
Why name it that way though?
The name was deliberately renamed as elf was imprecise (won't be the case for macOS). I renamed it because I knew the rest of deterministic_if will change anyway, e.g. /home/marxin/.wasmer/cache/compiled/llvm-opta-exp_art-non_vol_mem-pic-ro_ftable-v23).
Actually, skeptical if this should go into the deterministic engine id at all ... the artifact format.
Why? For the clean separation of the new formats (on disk), I see it necessary.
There was a problem hiding this comment.
It mixes up what the engine is vs what the artifact format is. That could become complicated for cache implementations, including eg our own.
It could eg make sense to have a separate function for the artifact format on the engine, but I wouldn't mix them up into the engine core deterministic ID.
They are only partially related.
I wouldn't care about the artifact format when just wanting to know what exactly that engine is.
There was a problem hiding this comment.
Pull request overview
Ensures compiler deterministic_id() strings incorporate configuration flags that can change generated machine code (or compiled artifact format), so module/artifact caches don鈥檛 accidentally reuse incompatible compiled outputs.
Changes:
- Singlepass: extend
deterministic_id()to include experimental artifact format, NaN canonicalization, and unaligned-memory settings. - LLVM: extend
deterministic_id()to include additional codegen-affecting flags (NaN canonicalization, non-volatile memops, PIC, readonly funcref table) while keeping opt level encoded. - Cranelift: extend
deterministic_id()to include opt level, experimental artifact format, NaN canonicalization, PIC, and unaligned-memory settings; adjust config field visibility to support this.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| lib/compiler-singlepass/src/compiler.rs | Adds config-sensitive components to Singlepass deterministic_id() for cache correctness. |
| lib/compiler-llvm/src/config.rs | Makes is_pic pub(crate) so LLVM compiler can encode it into the deterministic ID. |
| lib/compiler-llvm/src/compiler.rs | Expands LLVM deterministic_id() to include additional codegen-affecting config flags. |
| lib/compiler-cranelift/src/config.rs | Makes additional config fields pub(crate) so Cranelift compiler can encode them into the deterministic ID. |
| lib/compiler-cranelift/src/compiler.rs | Expands Cranelift deterministic_id() to include opt level and other codegen-sensitive flags. |
We must ensure all the config values that influence the code generation are reflected in the compiled artifact location.
Fixes: #6869