[Caml-list] flambda seemingly missing easy optimization

[Caml-list] flambda seemingly missing easy optimization

[Reed Wilson]

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

Hi list,

I've been playing around with the flambda build of 4.03.0+dev16, and I
found some seemingly useless code in the generated assembly and cmm tree
that is not returned without flambda.

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 non-flambda cmm is:
(function camlTest__compare_str_sub_1199
     (p/1200: val s/1201: val s_off/1202: val len/1203: val)
 (catch
   (if (< s_off/1202 1) (exit 2)
     (if
       (> (+ (+ s_off/1202 len/1203) -1)
         (+
           (<<
             (let tmp/1206 (- (<< (>>u (load int (+a s/1201 -8)) 10) 3) 1)
               (- tmp/1206 (load unsigned int8 (+a s/1201 tmp/1206))))
             1)
           1))
       (exit 2) (extcall "caml_string_equal" p/1200 s/1201 val)))
 with(2) (app "camlPervasives__invalid_arg_1007" "camlTest__1" val)))

flambda results in:
(function camlTest__compare_str_sub_4
     (p_9/1210: val s_8/1209: val s_off_7/1208: val len_6/1207: val)
 (catch
   (catch
     (if (< s_off_7/1208 1) (if (!= 3 1) (exit 2) (exit 3))
       (if
         (!=
           (let
             Pintcomp_arg_15/1217
               (+
                 (<<
                   (let
                     tmp/1221
                       (- (<< (>>u (load int (+a s_8/1209 -8)) 10) 3) 1)
                     (- tmp/1221 (load unsigned int8 (+a s_8/1209
tmp/1221))))
                   1)
                 1)
             (+
               (<<
                 (> (+ (+ s_off_7/1208 len_6/1207) -1) Pintcomp_arg_15/1217)
                 1)
               1))
           1)
         (exit 2) (exit 3)))
   with(3) (extcall "caml_string_equal" p_9/1210 s_8/1209 val))
 with(2)
   (app{test.ml:18,52-90} "camlPervasives__invalid_arg_279"
     "camlTest__apply_arg_31" val)))

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

-- 
ç

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

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

[Pierre Chambart]

[-- 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 --]

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

[Reed Wilson]

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

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

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

[Gabriel Scherer]

[-- 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 --]