MIME-Version: 1.0
References: <206b001d-1884-4081-bd02-bed5c92f02ba@eisentraut.org>
 <CAN55FZ3bsxQYEbmUqGhWaBf0ixmxGoLdijz75AgzMG=23M976g@mail.gmail.com>
 <CAN55FZ1JEotkU8TTwhNreUp0cTbY1ag6uOGbHJGSeqotGNUB5w@mail.gmail.com>
 <CA+hiPX8R69OeApj55Fy1VXOhuHUKfdvp2f80icLy65z76Jy=JA@mail.gmail.com>
 <CAN55FZ2qGH8WsRaX3UxC4E-ZEwv0ZHr9p1XQfLqLs1fGKLAaoA@mail.gmail.com>
In-Reply-To: 
 <CAN55FZ2qGH8WsRaX3UxC4E-ZEwv0ZHr9p1XQfLqLs1fGKLAaoA@mail.gmail.com>
From: Diego Fronza <diego.fronza@percona.com>
Date: Wed, 12 Mar 2025 10:39:19 -0300
Message-ID: 
 <CA+hiPX9ZHrryqFnMWfEZhqS4fLj2yv_shBR=cW07WXnSrDu8jA@mail.gmail.com>
Subject: Re: meson vs. llvm bitcode files
To: Nazir Bilal Yavuz <byavuz81@gmail.com>
Cc: Peter Eisentraut <peter@eisentraut.org>,
 pgsql-hackers <pgsql-hackers@postgresql.org>,
	Andres Freund <andres@anarazel.de>
Content-Type: multipart/alternative; boundary="000000000000c65bb30630255505"
Archived-At: 
 <https://www.postgresql.org/message-id/CA%2BhiPX9ZHrryqFnMWfEZhqS4fLj2yv_shBR%3DcW07WXnSrDu8jA%40mail.gmail.com>
Precedence: bulk

--000000000000c65bb30630255505
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

The v7 patch looks good to me, handling the bitcode modules in a uniform
way and also avoiding the hacky code and warnings, much better now.

A small note about the bitcode emission for generated sources in contrib,
using cube as example, currently it creates two dict entries in a list:
bc_seg_gen_sources =3D [{'srcfiles': [seg_scan]}]
bc_seg_gen_sources +=3D {'srcfiles': [seg_parse[0]]}

Then pass it to the bitcode_modules:
bitcode_modules +=3D {
  ...
  'gen_srcfiles': bc_seg_gen_sources,
}

It could be passed as a list with a single dict, since both generated
sources share the same compilation flags:
bitcode_modules +=3D {
  ...
  'gen_srcfiles': [
    {  'srcfiles': [cube_scan, cube_parse[0]] }.
  ]
}

Both approaches work, the first one has the advantage of being able to pass
separate additional_flags per generated source.

Thanks for your reply Nazir, also waiting for more opinions on this.

Regards,
Diego

On Wed, Mar 12, 2025 at 7:27=E2=80=AFAM Nazir Bilal Yavuz <byavuz81@gmail.c=
om>
wrote:

> Hi,
>
> On Tue, 11 Mar 2025 at 01:04, Diego Fronza <diego.fronza@percona.com>
> wrote:
> > I did a full review on the provided patches plus some tests, I was able
> to validate that the loading of bitcode modules is working also JIT works
> for both backend and contrib modules.
>
> Thank you!
>
> > To test JIT on contrib modules I just lowered the costs for all jit
> settings and used the intarray extension, using the data/test__int.data:
> > CREATE EXTENSION intarray;
> > CREATE TABLE test__int( a int[] );1
> > \copy test__int from 'data/test__int.data'
> >
> > For queries any from line 98+ on contrib/intarray/sql/_int.sql will wor=
k.
> >
> > Then I added extra debug messages to llvmjit_inline.cpp on
> add_module_to_inline_search_path() function, also on
> llvm_build_inline_plan(), I was able to see many functions in this module
> being successfully inlined.
> >
> > I'm attaching a new patch based on your original work which add further
> support for generating bitcode from:
>
> Thanks for doing that!
>
> >  - Generated backend sources: processed by flex, bison, etc.
> >  - Generated contrib module sources,
>
> I think we do not need to separate these two.
>
>    foreach srcfile : bitcode_module['srcfiles']
> -    if meson.version().version_compare('>=3D0.59')
> +    srcfilename =3D '@0@'.format(srcfile)
> +    if srcfilename.startswith('<CustomTarget')
> +      srcfilename =3D srcfile.full_path().split(meson.build_root() + '/'=
)[1]
> +    elif meson.version().version_compare('>=3D0.59')
>
> Also, checking if the string starts with '<CustomTarget' is a bit
> hacky, and 'srcfilename =3D '@0@'.format(srcfile)' causes a deprecation
> warning. So, instead of this we can process all generated sources like
> how generated backend sources are processed. I updated the patch with
> that.
>
> > On this patch I just included fmgrtab.c and src/backend/parser for the
> backend generated code.
> > For contrib generated sources I added contrib/cube as an example.
>
> I applied your contrib/cube example and did the same thing for the
> contrib/seg.
>
> > All relevant details about the changes are included in the patch itself=
.
> >
> > As you may know already I also created a PR focused on llvm bitcode
> emission on meson, it generates bitcode for all backend and contribution
> modules, currently under review by some colleagues at Percona:
> https://github.com/percona/postgres/pull/103
> > I'm curious if we should get all or some of the generated backend
> sources compiled to bitcode, similar to contrib modules.
>
> I think we can do this. I added other backend sources like you did in
> the PR but attached it as another patch (0007) because I wanted to
> hear other people's opinions on that first.
>
> v3 is attached.
>
> --
> Regards,
> Nazir Bilal Yavuz
> Microsoft
>

--000000000000c65bb30630255505
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div>Hi,</div><div><br></div><div>The v7 patch looks good =
to me, handling the bitcode modules in a uniform way and also avoiding the =
hacky code and warnings, much better now.</div><div><br></div><div>A small =
note about the bitcode emission for generated sources in contrib, using cub=
e as example, currently it creates two dict entries in a list:</div>bc_seg_=
gen_sources =3D [{&#39;srcfiles&#39;: [seg_scan]}]<div>bc_seg_gen_sources +=
=3D {&#39;srcfiles&#39;: [seg_parse[0]]}</div><div><br></div><div>Then pass=
 it to the bitcode_modules:</div><div>bitcode_modules +=3D {</div><div>=C2=
=A0 ...</div><div>=C2=A0 &#39;gen_srcfiles&#39;: bc_seg_gen_sources,</div><=
div>}</div><div><br><div>It could be passed as a list with a single dict, s=
ince both generated sources share=C2=A0the same compilation flags:</div><di=
v dir=3D"ltr"><div>bitcode_modules +=3D {</div><div>=C2=A0 ...</div><div>=
=C2=A0 &#39;gen_srcfiles&#39;: [</div><div>=C2=A0 =C2=A0 {=C2=A0 &#39;srcfi=
les&#39;: [cube_scan, cube_parse[0]] }.</div><div>=C2=A0 ]</div><div>}</div=
><div><br></div><div>Both approaches work, the first one has the advantage =
of being able to pass separate additional_flags per generated source.</div>=
<div><br></div><div>Thanks for your reply Nazir, also waiting for more opin=
ions on this.</div><div><br></div><div>Regards,</div><div>Diego</div><br cl=
ass=3D"gmail-Apple-interchange-newline"></div><div class=3D"gmail_quote gma=
il_quote_container"><div dir=3D"ltr" class=3D"gmail_attr">On Wed, Mar 12, 2=
025 at 7:27=E2=80=AFAM Nazir Bilal Yavuz &lt;<a href=3D"mailto:byavuz81@gma=
il.com">byavuz81@gmail.com</a>&gt; wrote:<br></div><blockquote class=3D"gma=
il_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,2=
04,204);padding-left:1ex">Hi,<br>
<br>
On Tue, 11 Mar 2025 at 01:04, Diego Fronza &lt;<a href=3D"mailto:diego.fron=
za@percona.com" target=3D"_blank">diego.fronza@percona.com</a>&gt; wrote:<b=
r>
&gt; I did a full review on the provided patches plus some tests, I was abl=
e to validate that the loading of bitcode modules is working also JIT works=
 for both backend and contrib modules.<br>
<br>
Thank you!<br>
<br>
&gt; To test JIT on contrib modules I just lowered the costs for all jit se=
ttings and used the intarray extension, using the data/test__int.data:<br>
&gt; CREATE EXTENSION intarray;<br>
&gt; CREATE TABLE test__int( a int[] );1<br>
&gt; \copy test__int from &#39;data/test__int.data&#39;<br>
&gt;<br>
&gt; For queries any from line 98+ on contrib/intarray/sql/_int.sql will wo=
rk.<br>
&gt;<br>
&gt; Then I added extra debug messages to llvmjit_inline.cpp on add_module_=
to_inline_search_path() function, also on llvm_build_inline_plan(), I was a=
ble to see many functions in this module being successfully inlined.<br>
&gt;<br>
&gt; I&#39;m attaching a new patch based on your original work which add fu=
rther support for generating bitcode from:<br>
<br>
Thanks for doing that!<br>
<br>
&gt;=C2=A0 - Generated backend sources: processed by flex, bison, etc.<br>
&gt;=C2=A0 - Generated contrib module sources,<br>
<br>
I think we do not need to separate these two.<br>
<br>
=C2=A0 =C2=A0foreach srcfile : bitcode_module[&#39;srcfiles&#39;]<br>
-=C2=A0 =C2=A0 if meson.version().version_compare(&#39;&gt;=3D0.59&#39;)<br=
>
+=C2=A0 =C2=A0 srcfilename =3D &#39;@0@&#39;.format(srcfile)<br>
+=C2=A0 =C2=A0 if srcfilename.startswith(&#39;&lt;CustomTarget&#39;)<br>
+=C2=A0 =C2=A0 =C2=A0 srcfilename =3D srcfile.full_path().split(meson.build=
_root() + &#39;/&#39;)[1]<br>
+=C2=A0 =C2=A0 elif meson.version().version_compare(&#39;&gt;=3D0.59&#39;)<=
br>
<br>
Also, checking if the string starts with &#39;&lt;CustomTarget&#39; is a bi=
t<br>
hacky, and &#39;srcfilename =3D &#39;@0@&#39;.format(srcfile)&#39; causes a=
 deprecation<br>
warning. So, instead of this we can process all generated sources like<br>
how generated backend sources are processed. I updated the patch with<br>
that.<br>
<br>
&gt; On this patch I just included fmgrtab.c and src/backend/parser for the=
 backend generated code.<br>
&gt; For contrib generated sources I added contrib/cube as an example.<br>
<br>
I applied your contrib/cube example and did the same thing for the contrib/=
seg.<br>
<br>
&gt; All relevant details about the changes are included in the patch itsel=
f.<br>
&gt;<br>
&gt; As you may know already I also created a PR focused on llvm bitcode em=
ission on meson, it generates bitcode for all backend and contribution modu=
les, currently under review by some colleagues at Percona: <a href=3D"https=
://github.com/percona/postgres/pull/103" rel=3D"noreferrer" target=3D"_blan=
k">https://github.com/percona/postgres/pull/103</a><br>
&gt; I&#39;m curious if we should get all or some of the generated backend =
sources compiled to bitcode, similar to contrib modules.<br>
<br>
I think we can do this. I added other backend sources like you did in<br>
the PR but attached it as another patch (0007) because I wanted to<br>
hear other people&#39;s opinions on that first.<br>
<br>
v3 is attached.<br>
<br>
--<br>
Regards,<br>
Nazir Bilal Yavuz<br>
Microsoft<br>
</blockquote></div></div></div>

--000000000000c65bb30630255505--