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
>



-- 
ç