From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Original-To: caml-list@sympa.inria.fr Delivered-To: caml-list@sympa.inria.fr Received: from mail2-relais-roc.national.inria.fr (mail2-relais-roc.national.inria.fr [192.134.164.83]) by sympa.inria.fr (Postfix) with ESMTPS id 2C6B67EE34 for ; Thu, 31 Mar 2016 21:30:22 +0200 (CEST) IronPort-PHdr: 9a23:mkRZmRQbLOLMIY6Y34HYh8/w8Npsv+yvbD5Q0YIujvd0So/mwa64bBaN2/xhgRfzUJnB7Loc0qyN4/CmAjRLuM3a+Fk5M7VyFDY9wf0MmAIhBMPXQWbaF9XNKxIAIcJZSVV+9Gu6O0UGUOz3ZlnVv2HgpWVKQka3CwN5K6zPF5LIiIzvjqbpq82VOF0D3WHmKZpJbzyI7izp/vEMhoVjLqtjgjDomVBvP9ps+GVzOFiIlAz97MrjtLRq8iBXpu5zv5UYCfayV+0CQLdZFDUrNXwurI2u7EGbDFjH2nxJaGgbnlJ3Agze6xeyCpz8vir+suNV1yyTPMmwRrcxD2eM9aBuHTDhgj0GOjpxy2rXh9Z9luoPrxurvR1yx8jPa4GYLvdkVqzYdNIeA2FGW5ACBGR6HoqgYt5XXKI6NuFCoty4/gNWoA== Authentication-Results: mail2-smtp-roc.national.inria.fr; spf=None smtp.pra=gabriel.scherer@gmail.com; spf=Pass smtp.mailfrom=gabriel.scherer@gmail.com; spf=None smtp.helo=postmaster@mail-io0-f173.google.com Received-SPF: None (mail2-smtp-roc.national.inria.fr: no sender authenticity information available from domain of gabriel.scherer@gmail.com) identity=pra; client-ip=209.85.223.173; receiver=mail2-smtp-roc.national.inria.fr; envelope-from="gabriel.scherer@gmail.com"; x-sender="gabriel.scherer@gmail.com"; x-conformance=sidf_compatible Received-SPF: Pass (mail2-smtp-roc.national.inria.fr: domain of gabriel.scherer@gmail.com designates 209.85.223.173 as permitted sender) identity=mailfrom; client-ip=209.85.223.173; receiver=mail2-smtp-roc.national.inria.fr; envelope-from="gabriel.scherer@gmail.com"; x-sender="gabriel.scherer@gmail.com"; x-conformance=sidf_compatible; x-record-type="v=spf1" Received-SPF: None (mail2-smtp-roc.national.inria.fr: no sender authenticity information available from domain of postmaster@mail-io0-f173.google.com) identity=helo; client-ip=209.85.223.173; receiver=mail2-smtp-roc.national.inria.fr; envelope-from="gabriel.scherer@gmail.com"; x-sender="postmaster@mail-io0-f173.google.com"; x-conformance=sidf_compatible X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: A0CLAQBWev1WjK3fVdFdhAd9BrkAhA4jhTA6AoE+BzsRAQEBAQEBAQERAQEBCA0JCR8xgi2CFAEBAQMBEhEdARsdAQMBCwYDAgsDCioCAiEBAREBBQEcBhMih28BAwoIDpR/j0GBMT4xizaBaoJXhnEKGScNUYQcAQEBAQEBAQMBAQEBAQEBARIBBQoFhg+ERoJAgWw/glOCVgWTEYQwMYFShCGFXkKBdYF9N4xZh0GGFxEegQ42giMegVQ6MAWIaAEBAQ X-IPAS-Result: A0CLAQBWev1WjK3fVdFdhAd9BrkAhA4jhTA6AoE+BzsRAQEBAQEBAQERAQEBCA0JCR8xgi2CFAEBAQMBEhEdARsdAQMBCwYDAgsDCioCAiEBAREBBQEcBhMih28BAwoIDpR/j0GBMT4xizaBaoJXhnEKGScNUYQcAQEBAQEBAQMBAQEBAQEBARIBBQoFhg+ERoJAgWw/glOCVgWTEYQwMYFShCGFXkKBdYF9N4xZh0GGFxEegQ42giMegVQ6MAWIaAEBAQ X-IronPort-AV: E=Sophos;i="5.24,423,1454972400"; d="scan'208,217";a="211462456" Received: from mail-io0-f173.google.com ([209.85.223.173]) by mail2-smtp-roc.national.inria.fr with ESMTP/TLS/AES128-GCM-SHA256; 31 Mar 2016 21:30:20 +0200 Received: by mail-io0-f173.google.com with SMTP id a129so118374502ioe.0 for ; Thu, 31 Mar 2016 12:30:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=I/h/UoAyViDHTy/FB4cn9O+N5qZzRF7IoqvwCE5MFww=; b=n6i2e7mKrlKDRJNANNbqDOFHvXigdJ4ZtIpVtmldzZg5QFqqf+Cn9TFiwfTdTmCfin QJcKDSkH+NiHu8LRpXHDz2K+8++Ff9s7iXMzR341NLtBQ6mdKHiTOTgVpKvNlen2OxCU MKHcZHeGcd1KRO958zbMU9WHp7iIHmb3gEUxOWHPp1CByYrt2OpeH/6+47OrKxkMTf7f k1pPf18dB1EWPpM5027x8XO/sE4d0HdKzmdb4vkX22ktgfcikMywmYVZQUZuJYj+Q4/R tYXHhqVtpkhkAofcESC3QfxgYIcKbZfyATp1exTynEWwM8PqvGrozGRuoZlh7xtq9JVM KOgQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=I/h/UoAyViDHTy/FB4cn9O+N5qZzRF7IoqvwCE5MFww=; b=fzhb4AyehuDHEvxJXpNrZnDsD9y8VIPiaIZhHmTU6o9SKp8eECQv9VLufN1FB5A3HG IgsE1Kn7TYbH4RfYhRSK9gizpxcC5/7mlv7Q/kv0I4akMaz7yjcvw+SwoLBCoaJR3YXj SltZj4j5Ln1m3hGAtdR4FllP/SiUlL0CpA0ehKCgHKC4Gqw71HpEvvQdP/WSyDWFlQb6 /lz/8A1C3UB2ZzRjiL1w75xsBazzoivk/Q5za0OWaCxgoKFxfaURZNHrA0wwUqm/a6ot a6Szj+dddVvcsz3uJrBYsUV4dQugydPNMP4u4krWQsy/f/X9equBji8JbFLwv0HJtG3r lqug== X-Gm-Message-State: AD7BkJIcAgsq6mfd4AhkgR8wR8UGFPuU4zkVrHi6eLG52BvCaTKMMIfEC3tF1+cTIpaMz4MOsTKDiJO1233n+A== X-Received: by 10.107.169.32 with SMTP id s32mr542593ioe.19.1459452619544; Thu, 31 Mar 2016 12:30:19 -0700 (PDT) MIME-Version: 1.0 Received: by 10.79.17.4 with HTTP; Thu, 31 Mar 2016 12:29:40 -0700 (PDT) In-Reply-To: References: <56FBA198.9000804@ocamlpro.com> From: Gabriel Scherer Date: Thu, 31 Mar 2016 21:29:40 +0200 Message-ID: To: Reed Wilson Cc: Pierre Chambart , caml users Content-Type: multipart/alternative; boundary=001a11421c44f476d6052f5d4ae8 Subject: Re: [Caml-list] flambda seemingly missing easy optimization --001a11421c44f476d6052f5d4ae8 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On Thu, Mar 31, 2016 at 5:57 PM, Reed Wilson 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 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 =3D >> if s_off < 0 || s_off + len > String.length s >> then invalid_arg "Don't do that!"; >> else p =3D s >> >> ... >> >> >> The odd code is toward the beginning: (if (!=3D 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"=3D"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 generat= ion >> 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 so= me >> part where apparently lost in a merge conflict: >> https://github.com/ocaml/ocaml/pull/430/commits/355cf1d40b854711911ed332= e9472cbd231ffc78 >> >> 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 >> > > > > -- > =C3=A7 > --001a11421c44f476d6052f5d4ae8 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
On Thu, Mar 31, 2016 at 5:57 PM, Reed Wilson <cedilla@gma= il.com> wrote:
> PS. I = wasn't too sure if mantis was still the recommended way of reporting bu= gs now that github is up and running.

Yes, mantis remains the one and only bug tracker used for the OCaml di= stribution. 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 <cedill= a@gmail.com> wrote:
Thanks for the explanation. I was avoiding fla= mbda 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 recommen= ded way of reporting bugs now that github is up and running.

On Wed, Mar 30, 2016 at 2:51 AM, Pierre Chambart <<= a href=3D"mailto:pierre.chambart@ocamlpro.com" target=3D"_blank">pierre.cha= mbart@ocamlpro.com> wrote:
=20=20 =20=20=20=20 =20=20
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 =3D
if s_off < 0 || s_off + len > String.length s
then invalid= _arg "Don't do that!";
else p =3D s

...

The odd code is toward the beginning: (if (!=3D 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
=C2=A0 (if (< s_off_7/1208 0) 1
=C2=A0=C2=A0=C2=A0 (let (Pintcomp_arg_15/1217 (string.length s_8/1209))=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (> (+ s_off_7/1208 len_6/1207) Pintco= mp_arg_15/1217)))
=C2=A0 (apply* camlPervasives__invalid_arg_279=C2=A0 "camlHop__app= ly_arg_31")
=C2=A0 (caml_string_equal p_9/1210 s_8/1209))

and the one without

(if
=C2=A0 (|| (< s_off/1206 0)
=C2=A0=C2=A0=C2=A0 (> (+ s_off/1206 len/1207) (string.length s/1205)= ))
=C2=A0 (apply* camlPervasives__invalid_arg_1007
=C2=A0=C2=A0=C2=A0 "camlHop__1"=3D"Don't do that!&qu= ot;)
=C2=A0 (caml_string_equal p/1204 s/1205))) ))

You will notice this is almost identical, but that the '||' ope= rator is in the 'if then else' form for the flambda version. This is = due to the early conversion of '||' and '&&' to sim= plify 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



--
=C3=A7

--001a11421c44f476d6052f5d4ae8--