Fix a bug in aliaser for pipelines with concat + slice - #851
Conversation
|
I asked AI agent to try to narrow the initial condition I had and it came up with this: "The narrowing experiment turned into a root-cause revision. Blocking on !alias.assume_in_bounds (write-side only) initially failed cannot_alias — but the failing IR showed the read-side slice views were generated correctly and then destroyed later: simplify rewrites an offset-0 slice view into a crop_buffer, and crop_needed() (simplify.cc:2114) drops crops that are only call inputs. Kernels that read input extents (reductions, dot k-dims, the test's combine3) then see the full packed buffer → OOB. Offset slices survive only because offset views aren't crop-expressible. With deps.buffer_input added to crop_needed, the narrowed guard passes everything — and notably, the guard fully removed still passes all of slinky (but breaks ynn attention with NaN, so the write-side block is genuinely needed). One thing for the discussion: the crop_needed change touches slinky's implicit contract that call inputs "may be larger than requested" — ynnpack kernels already violate that contract wherever they read reduction extents from inputs, so I'd argue the fix is right, but it deserves a discussion." I think it makes sense, what do you think? |
|
Hmm, I wonder if this last thing might clear up the first:
Didn't we fix that? I think we read reduction bounds from the reduction description, which is an output buffer, not an input. |
|
it might be not exactly correct in details, but I think simplifier does lose some crops necessary for slices. Regardless, it's a real bug and we can submit a wider "fix" I had initially, if you think it's better/safer (or maybe you have some other suggestions). |
e0d70af to
8002e0b
Compare
|
I reverted the change to the simplifier, but the condition is wider now. PTAL. |
| pc.store_outputs_innermost(); | ||
| slice_a.store_outputs_innermost(); | ||
| slice_b.store_outputs_innermost(); | ||
| slice_c.store_outputs_innermost(); |
There was a problem hiding this comment.
I've been thinking about what to do about this, this is a brutal test to have in the repo IMO.
I think a better test would be to implement a new example for the attention operator, like we have for softmax, that actually implements the op, instead of an abstract reproducer. What do you think?
There was a problem hiding this comment.
I'm fine with that, but it still will need to have some specific schedule to hit the bug.
There was a problem hiding this comment.
Yes that's OK, I commented in the wrong place. My thinking is, it will be easier to understand the test/pipeline if it is a concrete thing, instead of an abstract pipeline, even if it is the same operation as this.
There was a problem hiding this comment.
I started to look into it and realized that in order to reproduce it the attention isn't enough, we need tiled attention and even there we need to replicate the schedule and maybe some strides/extents to reproduce it. It kind of seems like it's going to be even more complicated, so I'm not sure now.
|
I wonder where we are at with this after some other recent fixes to concat related issues? Is this still needed? |
2 participants
The test is created by AI based on the internal pipeline which was failing for me, it's a little convoluted and I tried to simplify it further, but without much success.