Verify instruction encodings #74
Conversation
|
Here we go. The first patch is the one that really matters in this commit. Second is just a little fix I found along the way. The last two are totally optional, so feel free to drop them -- it did seem suspicious that we could exit legalize() without encodings[] for all instructions (later passes seem to dereference encodings[] without any protection), but we're probably guaranteed to not do anything else if legalization failed. |
anholt
force-pushed
the
verify-encodings
branch
from
b671963
to
17ba57c
Compare
|
Thanks! I'll review this shortly. The design around unencoded instructions isn't completely crystallized yet. I think it is probably too strict to require every instruction to have an encoding — it would require the legalizer to clean up after itself. Instead, I expect we'll allow unencoded instructions to linger as a kind of 'ghost instructions'. The requirement will be that a proper encoded instruction is not allowed to depend on the result of an unencoded instruction, and unencoded instructions can't have side effects. I expect that unencoded instructions will be used to inform metadata like DWARF expressions or bailout information for a JIT compiler. It's for cases where metadata needs to describe how to compute values that have been optimized out. |
lib/cretonne/src/context.rs
Outdated
| @@ -54,7 +54,7 @@ impl Context { | |||
| /// The `TargetIsa` argument is currently unused, but the verifier will soon be able to also | |||
| /// check ISA-dependent constraints. | |||
| pub fn verify<'a, ISA: Into<Option<&'a TargetIsa>>>(&self, _isa: ISA) -> verifier::Result { | |||
| verifier::verify_context(&self.func, &self.cfg, &self.domtree) | |||
| verifier::verify_context(&self.func, &self.cfg, &self.domtree, _isa.into()) | |||
There was a problem hiding this comment.
The leading underscore on _isa serves to silence Rust's dead code warning. I remove them when I start using an argument.
| @@ -91,6 +91,7 @@ pub fn legalize_function(func: &mut Function, cfg: &mut ControlFlowGraph, isa: & | |||
| // unsound. Should we attempt to detect that? | |||
| if changed { | |||
| pos.set_position(prev_pos); | |||
| continue; | |||
lib/cretonne/src/legalizer/mod.rs
Outdated
| @@ -92,6 +92,8 @@ pub fn legalize_function(func: &mut Function, cfg: &mut ControlFlowGraph, isa: & | |||
| if changed { | |||
| pos.set_position(prev_pos); | |||
| continue; | |||
| } else { | |||
| func.encodings.ensure(inst); | |||
There was a problem hiding this comment.
This is a good invariant to enforce. I wonder if it is simpler and safer to call encodings.resize(dfg.num_insts()) after the legalizer pass, though?
The other legalizer cases have a continue after setting the position to double back, while this one didn't. Make sure that we do, in case another legalizer block is added after this one.
…ize(). If we generated new instructions as part of legalize, and the new instructions failed to legalize, we'd be left with a func.encodings[] that would panic when you dereferenced the inst.
anholt
force-pushed
the
verify-encodings
branch
from
17ba57c
to
ba17e31
Compare
|
I like both those changes. Done. |
|
Awesome. Thanks! |
No description provided.