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

[Pierre Chambart <pierre.chambart@ocamlpro.com>]

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

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: 3599 bytes --]