Re: [Caml-list] <DKIM> flambda seemingly missing easy optimization

[Gabriel Scherer <gabriel.scherer@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3177 bytes --]

On Thu, Mar 31, 2016 at 5:57 PM, Reed Wilson <cedilla@gmail.com> wrote:
> PS. I wasn't too sure if mantis was still the recommended way of
reporting bugs now that github is up and running.

Yes, mantis remains the one and only bug tracker used for the OCaml
distribution. Github is used for pull requests, but it's also possible to
send patches through mantis.

On Thu, Mar 31, 2016 at 5:57 PM, Reed Wilson <cedilla@gmail.com> wrote:

> Thanks for the explanation. I was avoiding flambda as soon as I found this
> since I thought it would be representative of the state of its
> optimization. Good to know that's not the case!
>
> I recompiled after adding that branch back to cmmgen.ml and the problem
> seems to have disappeared.
>
> Thanks again,
> Reed
>
> PS. I wasn't too sure if mantis was still the recommended way of reporting
> bugs now that github is up and running.
>
> On Wed, Mar 30, 2016 at 2:51 AM, Pierre Chambart <
> pierre.chambart@ocamlpro.com> wrote:
>
>> On 30/03/2016 01:06, Reed Wilson wrote:
>>
>> Hi list,
>>
>> I made a small function to demonstrate it:
>> let compare_str_sub p s s_off len =
>> if s_off < 0 || s_off + len > String.length s
>> then invalid_arg "Don't do that!";
>> else p = s
>>
>> ...
>>
>>
>> The odd code is toward the beginning: (if (!= 3 1) (exit 2) (exit 3))
>> I don't know a lot about cmm code, but it looks like something the
>> compiler should be able to optimize better. Fiddling with the flambda
>> optimization options doesn't seem to remove it.
>>
>> Is this just due to how new flambda is, or is there some other reason
>> that code makes it through?
>>
>> Thanks,
>> Reed Wlison
>>
>>
>> This is some kind of code that was introduced by Cmmgen:
>>
>> Here is the clambda generated by flambda
>>
>> (if
>>   (if (< s_off_7/1208 0) 1
>>     (let (Pintcomp_arg_15/1217 (string.length s_8/1209))
>>       (> (+ s_off_7/1208 len_6/1207) Pintcomp_arg_15/1217)))
>>   (apply* camlPervasives__invalid_arg_279  "camlHop__apply_arg_31")
>>   (caml_string_equal p_9/1210 s_8/1209))
>>
>> and the one without
>>
>> (if
>>   (|| (< s_off/1206 0)
>>     (> (+ s_off/1206 len/1207) (string.length s/1205)))
>>   (apply* camlPervasives__invalid_arg_1007
>>     "camlHop__1"="Don't do that!")
>>   (caml_string_equal p/1204 s/1205))) ))
>>
>> You will notice this is almost identical, but that the '||' operator is
>> in the 'if then else' form for the flambda version. This is due to the
>> early conversion of '||' and '&&' to simplify things in the middle end. The
>> downside being that some specific patterns recognized by the cmm generation
>> are not recognized anymore (hence generating stupid stuff). There was a
>> pull request to add a few patterns to cmmgen to handle that case, but some
>> part where apparently lost in a merge conflict:
>> https://github.com/ocaml/ocaml/pull/430/commits/355cf1d40b854711911ed332e9472cbd231ffc78
>>
>> Thanks for the report !
>>
>> For this kind of things, you should also open a ticket on mantis to keep
>> track of it. I will soon open a PR to fix this.
>> --
>> Pierre
>>
>
>
>
> --
> ç
>

[-- Attachment #2: Type: text/html, Size: 5533 bytes --]