cranelift-isle: New IR and revised overlap checks #5195
Conversation
github-actions
bot
added
cranelift
Subscribe to Label Action
This issue or pull request has been labeled: "cranelift", "isle"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
| @@ -102,6 +102,7 @@ pub mod overlap; | |||
| pub mod parser; | |||
| pub mod sema; | |||
| pub mod trie; | |||
| pub mod trie_again; | |||
There was a problem hiding this comment.
|
Printing errors with Miette is crashing for me, and quoting way too much of the source code when it doesn't crash, and I don't understand why. But I'm disabling the new "rule shadowed by more general higher-priority rule" errors anyway because I want to merge this before fixing them in the backends. Anyway, builds don't normally turn on Miette, so I'm hoping to merge this without getting Miette exactly right. Otherwise I think this is ready to merge so I'd appreciate review. By the way, the number of rules which currently can't fire due to higher-priority rules always matching first is:
Apparently there aren't any rules in any backends which attempt to constrain a binding site in two incompatible ways, like |
|
I have a plausible hypothesis about why I was having trouble with Miette. The Rather than fix any of that right now, I've just removed all my attempts to use Miette "properly" in this PR. Instead, the new errors are reported with basically the same simple text formatting whether Miette is enabled or not. |
There was a problem hiding this comment.
Thanks for chasing this idea -- this seems really really promising! Overall this is a really clean implementation.
I only started to get a bit confused when reading through normalize_equivalence_classes and I'm wondering if there's a better way; at the very least, the implementation needs to be explained a lot better. But with that fixed, the rest LGTM.
|
As a data point, this PR has passed about 11 million executions of ISLE's fuzzer (when combined with #5236). |
I had tried to use Miette "right" and made things worse somehow. Among other changes, revert all my changes to unrelated parts of `error.rs` and `error_miette.rs`.
This saves about 50 lines of code in the trie_again module. The union-find implementation is about twice as long as that, counting comments and doc-tests, but that's a worth-while tradeoff. However, this makes `normalize_equivalence_classes` slower, because now finding all elements of an equivalence class takes time linear in the total size of all equivalence classes. If that ever turns out to be a problem in practice we can find some way to optimize `remove_set_of`.
We want to enforce that consumers of this representation can't observe non-deterministic ordering in any of its public types.
I'm not sure whether this is a good idea. It doesn't make the logic particularly simpler, and I think it will do more work if three or more binding sites with enum-variant constraints get set equal to each other.
jameysharp
force-pushed
the
isle-new-ir
branch
from
0b9fe90 to
b04880b
Compare
I've replaced the overlap checker with one that uses my new IR for ISLE. This is a step toward using the new IR for codegen too. This step gives me some confidence in the correctness of the IR construction without having to get all the codegen details right yet.
This isn't quite ready to merge yet, but it's at a milestone where I think it should pass CI, so I thought folks like @elliottt or @fitzgen might want to take a look.