[Mesa-dev] [PATCH 2/2] panfrost: Implement custom block/instruction iterators

[Alyssa Rosenzweig]

> > Could you explain a little bit more why this is neded? What do these
> > helpers do that the generic list.h helpers can't?
> 
> The _safe attribute in generic helpers is just about handling the case
> where the current item is removed from the list (or a new one is
> inserted and you want to skip the new entry in your iteration). Some
> users of the mir_foreach_xxx() iterators break this rule: they remove
> entries beyond the current one. That's okay if the removed entry is
> after the one stored in the temporary 'storage' param (also called
> next/prev sometimes), but when one actually removes the next/prev
> entry, the safe iterator ends up manipulating an element that's been
> removed from the list (->{prev,next} might be invalid) or even worse,
> something that's been released (use-after-free).

That makes sense; I wish I had been aware of this when writing the
compiler to begin with. Thanks for the clarification!
> 
> > Would it be possible
> > to extend the shared list.h helpers (might this contain functionality
> > interesting to other drivers as well)?
> 
> Hm, not without making them more complex I'm afraid. And since
> most users are happy with the current implementation I'm not entirely
> sure there's a need for such a feature.

Alright, just wanted to ask to make sure we've thought through this
fully before doing our own thing. Sounds good to me!

> > Maybe we want a dedicated variant _with_next() to save the
> > empty argument in the usual case (when we don't actually need it).
> 
> We can, but I also find it useful to expose this implementation detail
> to users, so that they have a rough idea of what's happening behind the
> scene (temporary storage pointing to the next instruction to allow
> safe removal of the current item).

I disagree. Writing a compiler, let alone a compiler for an architecture
like Midgard, is extremely challenging mentally, requiring the author to
balance a large number of layers of abstraction.

Minor implementation details of how linked lists are stored cannot be
one of them.

For v2, could you keep the API exposed to the rest of the compiler the
same (with only two arguments, no next), simply adding additional
_with_next() versions where necessary? Thank you.

-A
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190813/3b6bfb9f/attachment.sig>