ocaml-flambda / flambda-backend

The Flambda backend project for OCaml
93 stars 67 forks source link

Zero alloc: "assert all" and signatures #2687

Closed gretay-js closed 2 weeks ago

gretay-js commented 3 weeks ago

Top-level annotation [@@@zero_alloc all] asserts that all functions in the file are "zero alloc". This annotation should be enough to satisfies zero_alloc annotation on signatures, but currently doesn't work. For example, this file:

[@@@zero_alloc all]
let add x y = x + y

fails to compile against the interface:

val add : int -> int -> int [@@zero_alloc]

This PR proposes to fix it, but ignores the arity (which is not sound) because I don't know how to get the arity in Typemod!

We have all the information in Builtin_attributes.get_zero_alloc_attribute but can't replace Default_zero_alloc with Check at that point, because we may find another attribute on the let and then end up with a wrong warning about duplicate attributes from Typecore.add_check_attribute.

ccasin commented 2 weeks ago

@gretay-js I pushed a commit that moves the fabrication of the attribute into let_bound_idents_with_modes_sorts_and_checks, where we do have the arity. Can you see if this makes sense?

I think the plan to do better inference will also solve this problem, but it's probably good to ship this as a stopgap until I can get to that.

ccasin commented 2 weeks ago

@gretay-js I pushed a commit that moves the fabrication of the attribute into let_bound_idents_with_modes_sorts_and_checks, where we do have the arity. Can you see if this makes sense?

I think the plan to do better inference will also solve this problem, but it's probably good to ship this as a stopgap until I can get to that.

One consequence of putting the fabrication there is that we'll also fabricate it on functions bound by a class let (see the callsite of let_bound_idents_with_modes_sorts_and_checks in typeclass.ml. I'm unsure if that's desirable, but we can easily add a parameter to this function to distinguish the call sites if needed - there are only those two.

gretay-js commented 2 weeks ago

let_bound_idents_with_modes_sorts_and_checks

great, thanks for fixing it! does it change the terms that get propagated down the compilation pipeline or only their approximation for the purpose of the inclusion check? If it changes the term, we need to be a bit more careful, and the corresponding code in the middle end can be removed.

One consequence of putting the fabrication there is that we'll also fabricate it on functions bound by a class let ...

If it doesn't change the terms, I don't think we need the flag.

ccasin commented 2 weeks ago

let_bound_idents_with_modes_sorts_and_checks

great, thanks for fixing it! does it change the terms that get propagated down the compilation pipeline or only their approximation for the purpose of the inclusion check? If it changes the term, we need to be a bit more careful, and the corresponding code in the middle end can be removed.

It does change the terms (in the sense that the lambda version of these functions will have the zero_alloc check on, and so I think we can delete that code).

gretay-js commented 2 weeks ago

ok, I'll make a follow up PR to simplify the relevant parts from lambda onwards. I don't think we need to distinguish the call cites of let_bound_idents_with_modes_sorts_and_checks , because this PR matches the existing behavior for classes.