search

egraphs-good / egglog

egraphs + datalog!
https://egraphs-good.github.io/egglog/
MIT License
458 stars 54 forks source link

Delete -naive flag and disallow lookup actions in rules #461

Open FTRobbin opened 2 weeks ago

FTRobbin commented 2 weeks ago

This PR fixes Issue #420. Lookup actions in rules will now cause a type error LookupInRuleDisallowed.

Move specifically, this PR:

  1. Removes -naive flag and related desugaring code due to being replaced by this change.

  2. Fixes 'fail' failing due to not being identified as global in the remove_global rewrite pass.

  3. Adds new positive and negative tests for this type error.

  4. Rewrites the existing tests for compatibility with the new type error.

codspeed-hq[bot] commented 2 weeks ago

CodSpeed Performance Report

Merging #461 will improve performances by ×43

Comparing haobinni-0904 (dc42cd3) with main (2c8d947)

Summary

⚡ 4 improvements ✅ 4 untouched benchmarks

🆕 1 new benchmarks

Benchmarks breakdown

Benchmark main haobinni-0904 Change
⚡ eggcc-extraction 4,469.1 ms 103.7 ms ×43
⚡ herbie 308.7 ms 289.4 ms +6.66%
⚡ lambda 159.2 ms 126.4 ms +25.98%
🆕 looking_up_global N/A 314.3 µs N/A
⚡ python_array_optimize 6.9 s 6.1 s +13.84%
saulshanabrook commented 2 weeks ago

I'm a little worried about the time improvements, especially for lambda... That one is so dramatic I worry that maybe the semantics of the example changed?

Seeing all the changes, I also worry about the degradation for UX, it seems just more unwieldy with this change.

I know you said that automated desuguring had some issues, but I am wondering if that could be used to at least addressost of these cases? Where there particular issues with it for some cases or just in general?