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 = [{'srcfiles': [seg_scan]}]
bc_seg_gen_sources += {'srcfiles': [seg_parse[0]]}
Then pass it to the bitcode_modules:
bitcode_modules += {
...
'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 += {
...
'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 AM Nazir Bilal Yavuz <byavu...@gmail.com>
wrote:
> Hi,
>
> On Tue, 11 Mar 2025 at 01:04, Diego Fronza <diego.fro...@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 work.
> >
> > 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('>=0.59')
> + srcfilename = '@0@'.format(srcfile)
> + if srcfilename.startswith('<CustomTarget')
> + srcfilename = srcfile.full_path().split(meson.build_root() + '/')[1]
> + elif meson.version().version_compare('>=0.59')
>
> Also, checking if the string starts with '<CustomTarget' is a bit
> hacky, and 'srcfilename = '@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
>
