From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <pierre.chambart@ocamlpro.com>
X-Original-To: caml-list@sympa.inria.fr
Delivered-To: caml-list@sympa.inria.fr
Received: from mail3-relais-sop.national.inria.fr (mail3-relais-sop.national.inria.fr [192.134.164.104])
	by sympa.inria.fr (Postfix) with ESMTPS id 8DFD27EE3F
	for <caml-list@sympa.inria.fr>; Wed, 30 Mar 2016 11:51:32 +0200 (CEST)
IronPort-PHdr: 9a23:y3A/TxCH8exKwhl3uqeIUyQJP3N1i/DPJgcQr6AfoPdwSP7zpMbcNUDSrc9gkEXOFd2CrakU26yM7Ou5Az1IyK3CmU5BWaQEbwUCh8QSkl5oK+++Imq/EsTXaTcnFt9JTl5v8iLzG0FUHMHjew+a+SXqvnYsExnyfTB4Ov7yUtaLyZ/nh6bopNaKOVQArQH+SI0xBS3+lR/WuMgSjNkqAYcK4TyNnEF1ff9Lz3hjP1OZkkW0zM6x+Jl+73YY4Kp5pIYTGZn9Kp41QLAQMzUiKGczrJnguBXCSQ+GznQZW2QS1BFPBl6BpBrzW5O0tirhqsJ83jObNIv4V+MaQzOnup9qRQXyhW8sNzc8+mjNloQklKNWugis4Rd/yoveaZuJHP11d6bZZckdA2FGW5ACBGR6HoqgYt5XXKI6NuFCotylqg==
Authentication-Results: mail3-smtp-sop.national.inria.fr; spf=None smtp.pra=pierre.chambart@ocamlpro.com; spf=None smtp.mailfrom=pierre.chambart@ocamlpro.com; spf=Pass smtp.helo=postmaster@redisdead.crans.org
Received-SPF: None (mail3-smtp-sop.national.inria.fr: no sender
  authenticity information available from domain of
  pierre.chambart@ocamlpro.com) identity=pra;
  client-ip=138.231.136.39;
  receiver=mail3-smtp-sop.national.inria.fr;
  envelope-from="pierre.chambart@ocamlpro.com";
  x-sender="pierre.chambart@ocamlpro.com";
  x-conformance=sidf_compatible
Received-SPF: None (mail3-smtp-sop.national.inria.fr: no sender
  authenticity information available from domain of
  pierre.chambart@ocamlpro.com) identity=mailfrom;
  client-ip=138.231.136.39;
  receiver=mail3-smtp-sop.national.inria.fr;
  envelope-from="pierre.chambart@ocamlpro.com";
  x-sender="pierre.chambart@ocamlpro.com";
  x-conformance=sidf_compatible
Received-SPF: Pass (mail3-smtp-sop.national.inria.fr: domain of
  postmaster@redisdead.crans.org designates 138.231.136.39 as
  permitted sender) identity=helo; client-ip=138.231.136.39;
  receiver=mail3-smtp-sop.national.inria.fr;
  envelope-from="pierre.chambart@ocamlpro.com";
  x-sender="postmaster@redisdead.crans.org";
  x-conformance=sidf_compatible; x-record-type="v=spf1"
X-IronPort-Anti-Spam-Filtered: true
X-IronPort-Anti-Spam-Result: A0CjAABZoftWiieI54pdhAa5aYIPAQ2BcCOFMDoCgUA4FAEBAQEBAQEBEAEBAQoWB1CCLYIVAQEEI1URCQIOCgkWCwICCQMCAQIBRQYBDAgCiCMFCZI0nReQeAEBCAEBAQEYBIYehEWELIMSglYFkw6EX4FShCCFXoQ0hzuFVY8QHgEBglaBU2+IOgEBAQ
X-IPAS-Result: A0CjAABZoftWiieI54pdhAa5aYIPAQ2BcCOFMDoCgUA4FAEBAQEBAQEBEAEBAQoWB1CCLYIVAQEEI1URCQIOCgkWCwICCQMCAQIBRQYBDAgCiCMFCZI0nReQeAEBCAEBAQEYBIYehEWELIMSglYFkw6EX4FShCCFXoQ0hzuFVY8QHgEBglaBU2+IOgEBAQ
X-IronPort-AV: E=Sophos;i="5.24,415,1454972400"; 
   d="scan'208,217";a="171617231"
Received: from redisdead.crans.org ([138.231.136.39])
  by mail3-smtp-sop.national.inria.fr with ESMTP/TLS/ADH-AES256-GCM-SHA384; 30 Mar 2016 11:51:22 +0200
Received: from [192.168.1.114] (unknown [134.157.22.155])
	(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits))
	(No client certificate requested)
	by redisdead.crans.org (Postfix) with ESMTPSA id F1ED01353;
	Wed, 30 Mar 2016 11:51:21 +0200 (CEST)
To: Reed Wilson <cedilla@gmail.com>, caml-list@inria.fr
References: <CALLFq5RC_=7EnyRap=P8GVwpQJVO1DnTZUh23Gf05NPeiCVKrw@mail.gmail.com>
From: Pierre Chambart <pierre.chambart@ocamlpro.com>
X-Enigmail-Draft-Status: N1110
Organization: OcamlPro
Message-ID: <56FBA198.9000804@ocamlpro.com>
Date: Wed, 30 Mar 2016 11:51:20 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Icedove/38.6.0
MIME-Version: 1.0
In-Reply-To: <CALLFq5RC_=7EnyRap=P8GVwpQJVO1DnTZUh23Gf05NPeiCVKrw@mail.gmail.com>
Content-Type: multipart/alternative;
 boundary="------------010003090908040509070901"
X-Validation-by: pierre.chambart@ocamlpro.com
Subject: Re: [Caml-list] <DKIM> flambda seemingly missing easy optimization

This is a multi-part message in MIME format.
--------------010003090908040509070901
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit

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

--------------010003090908040509070901
Content-Type: text/html; charset=utf-8
Content-Transfer-Encoding: 8bit

<html>
  <head>
    <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    <div class="moz-cite-prefix">On 30/03/2016 01:06, Reed Wilson wrote:<br>
    </div>
    <blockquote
cite="mid:CALLFq5RC_=7EnyRap=P8GVwpQJVO1DnTZUh23Gf05NPeiCVKrw@mail.gmail.com"
      type="cite">
      <div dir="ltr">Hi list,
        <div><br>
        </div>
        <div>I made a small function to demonstrate it:</div>
        <div>
          <div>
            <div>let compare_str_sub p s s_off len =</div>
            <div><span class="" style="white-space:pre">	</span>if s_off
              &lt; 0 || s_off + len &gt; String.length s</div>
            <div><span class="" style="white-space:pre">		then </span>invalid_arg
              "Don't do that!";</div>
            <div><span class="" style="white-space:pre">		else </span>p
              = s</div>
          </div>
          <div><br>
          </div>
        </div>
      </div>
    </blockquote>
    ...<br>
    <blockquote
cite="mid:CALLFq5RC_=7EnyRap=P8GVwpQJVO1DnTZUh23Gf05NPeiCVKrw@mail.gmail.com"
      type="cite">
      <div dir="ltr">
        <div><br>
          <div>The odd code is toward the beginning: (if (!= 3 1) (exit
            2) (exit 3))</div>
          <div>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.</div>
          <div><br>
          </div>
          <div>Is this just due to how new flambda is, or is there some
            other reason that code makes it through?</div>
          <div><br>
          </div>
          <div>Thanks,</div>
          <div>Reed Wlison</div>
        </div>
      </div>
    </blockquote>
    <br>
    This is some kind of code that was introduced by Cmmgen:<br>
    <br>
    Here is the clambda generated by flambda<br>
    <br>
    (if<br>
      (if (&lt; s_off_7/1208 0) 1<br>
        (let (Pintcomp_arg_15/1217 (string.length s_8/1209))<br>
          (&gt; (+ s_off_7/1208 len_6/1207) Pintcomp_arg_15/1217)))<br>
      (apply* camlPervasives__invalid_arg_279  "camlHop__apply_arg_31")<br>
      (caml_string_equal p_9/1210 s_8/1209))<br>
    <br>
    and the one without<br>
    <br>
    (if<br>
      (|| (&lt; s_off/1206 0)<br>
        (&gt; (+ s_off/1206 len/1207) (string.length s/1205)))<br>
      (apply* camlPervasives__invalid_arg_1007<br>
        "camlHop__1"="Don't do that!")<br>
      (caml_string_equal p/1204 s/1205))) ))<br>
    <br>
    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 '&amp;&amp;' 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:
<a class="moz-txt-link-freetext" href="https://github.com/ocaml/ocaml/pull/430/commits/355cf1d40b854711911ed332e9472cbd231ffc78">https://github.com/ocaml/ocaml/pull/430/commits/355cf1d40b854711911ed332e9472cbd231ffc78</a><br>
    <br>
    Thanks for the report !<br>
    <br>
    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.<br>
    -- <br>
    Pierre<br>
  </body>
</html>

--------------010003090908040509070901--