New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allocating intermediate floats when ignoring floats. #6606
Comments
Comment author: @alainfrisch How often does this happen in practice? I've attached a patch which should handle this specific case, but one could clearly do a much better job at optimizing the %ignore primitive (removing all pure computations). Have you noticed other similar cases not related to %ignore? |
Comment author: @diml The place where we noticed it originally was bin_prot. We have a set of combinators for computing the serialized size of a value, and the one for floats simply ignores its argument: https://github.com/janestreet/bin_prot/blob/master/lib/size.ml#L55 Before the hack it was just: let bin_size_float _ = 8 |
Comment author: @alainfrisch Thanks for the info. It's always useful when opening such a ticket to mention some real world case where it matters. I've uploaded an improved patch which also removes the memory load in that case. The problem is that I don't fully understand the "semantics" of Cmm.remove_unit. It seems to me that it is always used in contexts where we don't care about the result, only the side effects of the computation. But then it could be much more aggressive and avoid for instance the default case (c -> Csequence (c, Ctuple [])) (and use c -> c instead). Perhaps there are more invariants (such as not changing the "type" of the sub-expression), and I don't feel confident enough to merge the commit in its current form. |
Comment author: @diml I wonder if this could be done at the level of deadcode.ml. Currently it doesn't get rid of the allocation because of the test [Proc.op_is_pure] which is false for [Ialloc]. But intuitively I would say that it is safe to get rid of unused allocations. If [Ialloc] can be removed then I think it will get rid of the memory load automatically as well. |
Comment author: @alainfrisch I think it's not the Ialloc that should be declared as pure. Its result is used in init stores, so even if declared pure, it won't be discarded. It seems we should rather do something like declaring "Istore(_, _, false)" pure. Is that sound? |
Comment author: @diml BTW I just tried with flambda and the allocation is avoided in both [f1] and [f2], so we can just revisit this when flambda is merged. |
Comment author: @damiendoligez flambda is now merged and indeed it avoids allocation in both f1 and f2. |
Original bug ID: 6606
Reporter: roshanjames
Status: resolved (set by @damiendoligez on 2016-04-13T15:47:47Z)
Resolution: fixed
Priority: normal
Severity: minor
Target version: 4.03.0+dev / +beta1
Fixed in version: 4.03.0+dev / +beta1
Category: middle end (typedtree to clambda)
Monitored by: @diml @ygrek @jmeber yminsky @yakobowski
Bug description
let ignore1 (f : float) = ignore (f)
let ignore2 (f : float) = ignore(truncate f : int)
type t = {
x : float;
y : float;
}
let f1 t = ignore1 t.x
let f2 t = ignore2 t.x
In the above, f1 allocates while f2 does not. It would be nice if both of these did not allocate. FWIW, allocation of intermediate floats are hard to reason about and fixes like this would help make allocation profiles of functions more predictable.
File attachments
The text was updated successfully, but these errors were encountered: