Hi Andres,
> I think I'd probably try to apply this to master independent of the
> larger patchset, to avoid a large dependency.
Awesome! +1. Attached 2nd version of patch rebased on master.
(v2-0001-Resolve-PL-handler-names-for-JITed-code-instead-o.patch)
> Did you check whether there's any cases this fails in the tree with your
> patch applied? The way I usually do that is by running the regression
> tests like
> PGOPTIONS='-cjit_above_cost=0' make -s -Otarget check-world
>
> (which will take a bit longer if use an optimized LLVM build, and a
> *lot* longer if you use a debug llvm build)
Great suggestion! I used:
PGOPTIONS='-c jit_above_cost=0' gmake installcheck-world
It all passed except a couple of logical decoding tests that never pass
on my machine for any tree (t/
006_logical_decoding.pl and
t/
010_logical_decoding_timelines.pl) and point (which seems to be failing even
on master as of: d80be6f2f) I have attached the regression.diffs which captures
the point failure.
> Hm. Aren't you breaking things here? If fmgr_symbol returns a basename
> of NULL, as is the case for all internal functions, you're going to
> print a NULL pointer, no?
For internal functions, it is supposed to return modname = NULL but basename
will be non-NULL right? As things stand, fmgr_symbol can never return a null
basename. I have added an Assert to make that even more explicit.
> Cool! I'll probably merge that into my patch (with attribution of
> course).
>
> I wonder if it'd nicer to not have separate C variables for all of
> these, and instead look them up on-demand from the module loaded in
> llvm_create_types(). Not sure.
Great! It is much nicer indeed. Attached version 2 with your suggested changes.
(v2-0001-Rely-on-llvmjit_types-for-building-EvalFunc-calls.patch)
Used the same testing method as above.
> Sorry for not replying to that earlier. I'm not quite sure it's
> actually worthwhile doing so - did you try to measure any memory / cpu
> savings?
No problem, thanks for the reply! Unfortunately, I did not do anything
significant in terms of mem/cpu measurements. However, I have noticed non-trivial
differences between optimized and unoptimized .bc files that were dumped from
time to time.
> The magnitude of wins aside, I also have a local patch that I'm going to
> try to publish this or next week, that deduplicates functions more
> aggressively, mostly to avoid redundant optimizations. It's quite
> possible that we should run that before the function passes - or even
> give up entirely on the function pass optimizations. As the module pass
> manager does the same optimizations it's not that clear in which cases
> it'd be beneficial to run it, especially if it means we can't
> deduplicate before optimizations.
Agreed, excited to see the patch!
--
Soumyadeep
