refactor: Split ArrayBorrowCompiler off from ArrayGetitemCompiler#1303
refactor: Split ArrayBorrowCompiler off from ArrayGetitemCompiler#1303
Conversation
guppylang-internals/src/guppylang_internals/std/_internal/compiler/array.py
Outdated
Show resolved
Hide resolved
| (In the linear, non-owned, case they will be replaced afterwards.)""" | ||
|
|
||
| def _build_classical_getitem(self, array: Wire, idx: Wire) -> CallReturnWires: | ||
| """Constructs `__getitem__` for classical arrays.""" |
There was a problem hiding this comment.
This could become (borrow_)array::get following Quantinuum/tket2#1181 but looks like that's non-conflicting/orthogonal (yay!)
|
I am not sure I like this because in my opinion it muddles where we are borrow array specific or just generally reason about arrays in Guppy, but I would welcome other opinions. I don't think this is necessarily blocking to getting borrow arrays deployed as it is just a refactor (though not sure if breaking?) |
|
I was thinking before you wrote this that the first commit was maybe better - a smaller change just to make clearer what was already happening. I think your comment sounds like the same conclusion, so I've gone back to that, maybe have another look.
Well, I think all of this has become borrow-array specific now??
Shouldn't break anything, should just verify what's happening already, indeed, so might come later. (I felt I needed to understand what was going on before I could approve your PR, and having figured it out, it seemed like a good idea to make that more obvious...) |
acl-cqc
force-pushed
the
acl/array_borrow_getitem
branch
from
582778a to
927ca0c
Compare
Bencher Report
acl-cqc
force-pushed
the
acl/array_borrow_getitem
branch
from
927ca0c to
c1bc728
Compare
|
| Branch | acl/array_borrow_getitem |
| Testbed | Linux |
🚨 2 Alerts
| Benchmark | Measure Units | View | Benchmark Result (Result Δ%) | Upper Boundary (Limit %) |
|---|---|---|---|---|
| tests/benchmarks/test_big_array.py::test_big_array_compile | hugr_nodes nodes x 1e3 | 📈 plot 🚷 threshold 🚨 alert (🔔) | 6.59 x 1e3(+32.64%)Baseline: 4.97 x 1e3 | 5.02 x 1e3 (131.33%) |
| tests/benchmarks/test_big_array.py::test_big_array_compile | hugr_bytes bytes x 1e3 | 📈 plot 🚷 threshold 🚨 alert (🔔) | 135.90 x 1e3(+4.99%)Baseline: 129.44 x 1e3 | 130.74 x 1e3 (103.95%) |
Click to view all benchmark results
| Benchmark | hugr_bytes | Benchmark Result bytes x 1e3 (Result Δ%) | Upper Boundary bytes x 1e3 (Limit %) | hugr_nodes | Benchmark Result nodes (Result Δ%) | Upper Boundary nodes (Limit %) |
|---|---|---|---|---|---|---|
| tests/benchmarks/test_big_array.py::test_big_array_compile | 📈 view plot 🚷 view threshold 🚨 view alert (🔔) | 135.90 x 1e3(+4.99%)Baseline: 129.44 x 1e3 | 130.74 x 1e3 (103.95%) | 📈 view plot 🚷 view threshold 🚨 view alert (🔔) | 6,590.00(+32.64%)Baseline: 4,968.39 | 5,018.07 (131.33%) |
| tests/benchmarks/test_ctrl_flow.py::test_many_ctrl_flow_compile | 📈 view plot 🚷 view threshold | 19.77 x 1e3(-9.60%)Baseline: 21.86 x 1e3 | 22.08 x 1e3 (89.51%) | 📈 view plot 🚷 view threshold | 606.00(-11.94%)Baseline: 688.17 | 695.05 (87.19%) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1303 +/- ##
=======================================
Coverage 93.44% 93.44%
=======================================
Files 123 123
Lines 11432 11434 +2
=======================================
+ Hits 10683 10685 +2
Misses 749 749 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| """Compiler for the `array.__getitem__` function.""" | ||
| """Compiler for the `array.__getitem__` function, used for both classical and | ||
| linear arrays e.g. when pulling out arguments to pass to a function. | ||
| (In the linear, non-owned, case they will be replaced afterwards.)""" |
There was a problem hiding this comment.
I have just been looking at how this happens in the frontend - check_place_assignable filling in the getitem_call field of a SubscriptAccess - it is nontrivial, otherwise I would love to have separated out the two different kinds of GetItem (linear with setitem to follow, vs classical without) first
3 participants
The goal here is to (a) make it clearer what's going on, (b) give a bit more protection, via asserts, against changes elsewhere in the compiler accidentally generating bad code.
ArrayGetitemCompiler compiles classical and linear variants separately; and has two uses: for
fn __getitem__ofclass arrayand forfn _array_unsafe_getitem(used only inArrayIter).First thought was that these can now be separate: ArrayIter ends in
discard_all_borrowed, so just a borrow would suffice even in the classical case. In fact it turns out that the ArrayIter code only ever uses the linear case, because it's output as polymorphic in Hugr; so outputtinggetfor the classical case would be just as efficient - it never happens. But that would be quite misleading, I argue this refactor makes it clearer what's going on.Passes mypy. Not done any execution/integration tests yet...