Add CompilerConfig opt to disable IR verification in debug mode
#1332
Conversation
MarkMcCaskey
added
📦 lib-compiler-cranelift
MarkMcCaskey
force-pushed
the
feature/disable-ir-verification
branch
from
7d59fb4
to
403e14b
Compare
nlewycky
changed the title
CompilerConifg opt to disable IR verification in debug modeCompilerConfig opt to disable IR verification in debug mode
lib/clif-backend/src/lib.rs
Outdated
| enable_verifier = !config.disable_debug_mode_verification; | ||
| } else { | ||
| // Set defaults if no config found. | ||
| enable_verifier = true; |
There was a problem hiding this comment.
How about enable_verifier = cfg!(test) || cfg!(debug_assertions)?
There was a problem hiding this comment.
That would require duplicating that boolean expression because it needs to be checked on the other case too! I considered it but decided against it
lib/runtime-core/src/backend.rs
Outdated
| /// (for example in 'debug' builds). Enabling this flag will make compilation faster at the | ||
| /// cost of not detecting bugs in the compiler. The verification steps that this flag | ||
| /// disables are disabled by default in 'release' builds. | ||
| pub disable_debug_mode_verification: bool, |
There was a problem hiding this comment.
Is there anything stopping us from turning verification on in a release build? Say, for users who are prepared to spend extra compile-time for extra assurance? If we can do that, could we make this control the setting unilaterally, and also remove the negative?
There was a problem hiding this comment.
Yup, agreed with Nick. Since debug verification is probably always false, we can do something like:
| pub disable_debug_mode_verification: bool, | |
| pub enable_ir_verification: bool, |
(note: this can work for both Cranelift and LLVM, since they both have IR verifiers).
There was a problem hiding this comment.
I'd like to rename it to something like "extra_safety_checks" or "backend_internal_verification" or something along those lines. There's nothing special about clif/llvm here, singlepass could have its own safety checks too, and it doesn't have an IR.
There was a problem hiding this comment.
What about enable_internal_verification or enable_verification (I kind of like keeping backend out, since this might be a setting on the backend config itself?)
There was a problem hiding this comment.
It's phrased in the negative because we derive(Default) which means it defaults to false haha. I can make a manual implementation of Default though
lib/clif-backend/src/lib.rs
Outdated
| enable_verifier = cfg!(test) || cfg!(debug_assertions); | ||
| } | ||
|
|
||
| if enable_verifier { |
There was a problem hiding this comment.
Optional.
⛳ builder.set("enable_verifier", if enable_verifier { "true" } else { "false" }).unwrap();
| impl Default for CompilerConfig { | ||
| fn default() -> Self { | ||
| Self { | ||
| symbol_map: Default::default(), |
There was a problem hiding this comment.
Couldn't you use ..Default::default() for these?
There was a problem hiding this comment.
That would infinite loop I believe! My understanding of what ..Default::default() does is that it calls default on the type you're constructing and then overrides the fields you specified. Because we're defining default, I don't believe we can use that syntax here.
There was a problem hiding this comment.
Yeah, just verified this:
warning: function cannot return without recursing
--> lib/runtime-core/src/backend.rs:159:5
|
159 | fn default() -> Self {
| ^^^^^^^^^^^^^^^^^^^^ cannot return without recursing
...
178 | ..Default::default()
| ------------------ recursive call site
|
= note: `#[warn(unconditional_recursion)]` on by default
= help: a `loop` may express intention better if this is on purpose
|
bors r+ |
Timed out |
|
bors retry |
Build succeeded
|
We'll need this to test the patch wasmerio/wasmer#1332
Resolves #1330
Review