LLVM 16 (opaque pointers)

Oh, one important thing I forgot to mention: that patch is for LLVM 16
only, and I developed it with a local build of their "release/16.x"
branch on a FreeBSD box, and also tested with a released package for
16 on a Debian box. Further changes are already needed for their
"main" branch (LLVM 17-to-be), so this won't quite be enough to shut
seawasp up. At a glance, we will need to change from the "old pass
manager" API that has recently been vaporised[1]https://github.com/llvm/llvm-project/commit/0aac9a2875bad4f065367e4a6553fad78605f895
(llvm-c/Transforms/PassManagerBuilder.h) to the new one[2]https://llvm.org/docs/NewPassManager.html[3]https://reviews.llvm.org/D102136
(llvm-c/Transforms/PassBuilder.h), which I suspect/hope will be as
simple as changing llvmjit.c to call LLVMRunPasses() with a string
describing the passes we want in "opt -passes" format, instead of our
code that calls LLVMAddFunctionInlingPass() etc. But that'll be a
topic for another day, and another thread.

[1]: https://github.com/llvm/llvm-project/commit/0aac9a2875bad4f065367e4a6553fad78605f895
[2]: https://llvm.org/docs/NewPassManager.html
[3]: https://reviews.llvm.org/D102136

On Mon, May 22, 2023 at 03:38:44PM +1200, Thomas Munro wrote:
Further changes are already needed for their "main" branch (LLVM
17-to-be), so this won't quite be enough to shut seawasp up. At a
glance, we will need to change from the "old pass manager" API that
has recently been vaporised[1]
(llvm-c/Transforms/PassManagerBuilder.h) to the new one[2][3]
(llvm-c/Transforms/PassBuilder.h), which I suspect/hope will be as
simple as changing llvmjit.c to call LLVMRunPasses() with a string
describing the passes we want in "opt -passes" format, instead of our
code that calls LLVMAddFunctionInlingPass() etc. But that'll be a
topic for another day, and another thread.

[1] https://github.com/llvm/llvm-project/commit/0aac9a2875bad4f065367e4a6553fad78605f895
[2] https://llvm.org/docs/NewPassManager.html
[3] https://reviews.llvm.org/D102136

Thanks for tackling the topic! I've tested it with a couple of versions,
LLVM 12 that comes with my Gentoo box, LLVM 15 build from sources and
the modified version of patch adopted for LLVM 17 (build form sources as
well). In all three cases everything seems to be working fine.

Simple benchmarking with a query stolen from some other jit thread
(pgbench running single client with multiple unions of selects a-la
SELECT a, count(*), sum(b) FROM test WHERE c = 2 GROUP BY a) show some
slight performance differences, but nothing dramatic so far. LLVM 17
version produces the lowest latency, with faster generation, inlining
and optimization, but slower emission time. LLVM 12 version produces the
largest latencies with everything except emission timings being slower.
LLVM 15 is somewhere in between.

I'll continue reviewing and, for the records, attach adjustments I was
using for LLVM 17 (purely for testing, not taking into account other
versions), in case if I've missed something.

Le dimanche 21 mai 2023, 05:01:41 CEST Thomas Munro a écrit :

Hi,

Here is a draft version of the long awaited patch to support LLVM 16.
It's mostly mechanical donkeywork, but it took some time: this donkey
found it quite hard to understand the mighty getelementptr
instruction[1] and the code generation well enough to figure out all
the right types, and small mistakes took some debugging effort*. I
now finally have a patch that passes all tests.

Though it's not quite ready yet, I thought I should give this status
update to report that the main task is more or less complete, since
we're starting to get quite a few emails about it (mostly from Fedora
users) and there is an entry for it on the Open Items for 16 wiki
page. Comments/review/testing welcome.

Hello Thomas,

Thank you for this effort !

I've tested it against llvm 15 and 16, and found no problem with it.

6. I need to go through the types again with a fine tooth comb, and
check the test coverage to look out for eg GEP array arithmetic with
the wrong type/size that isn't being exercised.

I haven't gone through the test coverage myself, but I exercised the following
things:

- running make installcheck with jit_above_cost = 0
- letting sqlsmith hammer random queries at it for a few hours.

This didn't show obvious issues.

*For anyone working with this type of IR generation code and
questioning their sanity, I can pass on some excellent advice I got
from Andres: build LLVM yourself with assertions enabled, as they
catch some classes of silly mistake that otherwise just segfault

inscrutably on execution.

I tried my hand at backporting it to previous versions, and not knowing
anything about it made me indeed question my sanity. It's quite easy for PG
15, 14, 13. PG 12 is nothing insurmontable either, but PG 11 is a bit hairier
most notably due to to the change in fcinfo args representation. But I guess
that's also a topic for another day :-)

Best regards,

--
Ronan Dunklau

Hi,

On 2023-08-10 16:56:54 +0200, Ronan Dunklau wrote:

I tried my hand at backporting it to previous versions, and not knowing
anything about it made me indeed question my sanity. It's quite easy for PG
15, 14, 13. PG 12 is nothing insurmontable either, but PG 11 is a bit hairier
most notably due to to the change in fcinfo args representation. But I guess
that's also a topic for another day :-)

Given that 11 is about to be EOL, I don't think it's worth spending the time
to support a new LLVM version for it.

Greetings,

Andres Freund

Hi,

On 2023-05-21 15:01:41 +1200, Thomas Munro wrote:

*For anyone working with this type of IR generation code and
questioning their sanity, I can pass on some excellent advice I got
from Andres: build LLVM yourself with assertions enabled, as they
catch some classes of silly mistake that otherwise just segfault
inscrutably on execution.

Hm. I think we need a buildfarm animal with an assertion enabled llvm 16 once
we merge this. I think after an upgrade my buildfarm machine has the necessary
resources.

@@ -150,7 +150,7 @@ llvm_compile_expr(ExprState *state)

/* create function */
eval_fn = LLVMAddFunction(mod, funcname,
-							  llvm_pg_var_func_type("TypeExprStateEvalFunc"));
+							  llvm_pg_var_func_type("ExecInterpExprStillValid"));

Hm, that's a bit ugly. But ...

@@ -77,9 +80,44 @@ extern Datum AttributeTemplate(PG_FUNCTION_ARGS);
Datum
AttributeTemplate(PG_FUNCTION_ARGS)
{
+	PGFunction	fp PG_USED_FOR_ASSERTS_ONLY;
+
+	fp = &AttributeTemplate;
PG_RETURN_NULL();
}

Other parts of the file do this by putting the functions into
referenced_functions[], i'd copy that here and below.

+void
+ExecEvalSubroutineTemplate(ExprState *state,
+						   struct ExprEvalStep *op,
+						   ExprContext *econtext)
+{
+	ExecEvalSubroutine fp PG_USED_FOR_ASSERTS_ONLY;
+
+	fp = &ExecEvalSubroutineTemplate;
+}
+
+extern bool ExecEvalBoolSubroutineTemplate(ExprState *state,
+										   struct ExprEvalStep *op,
+										   ExprContext *econtext);
+bool
+ExecEvalBoolSubroutineTemplate(ExprState *state,
+							   struct ExprEvalStep *op,
+							   ExprContext *econtext)
+{
+	ExecEvalBoolSubroutine fp PG_USED_FOR_ASSERTS_ONLY;
+
+	fp = &ExecEvalBoolSubroutineTemplate;
+	return false;
+}
+

Thanks for working on this!

Greetings,

Andres Freund

[...] Further changes are already needed for their "main" branch (LLVM
17-to-be), so this won't quite be enough to shut seawasp up.

For information, the physical server which was hosting my 2 bf animals
(seawasp and moonjelly) has given up rebooting after a power cut a few
weeks/months ago, and I have not setup a replacement (yet).

--
Fabien.

Belated thanks Dmitry, Ronan, Andres for your feedback. Here's a new
version, also including Dmitry's patch for 17 which it is now also
time to push. It required a bit more trivial #if magic to be
conditional, as Dmitry already mentioned. I just noticed that Dmitry
had the LLVMPassBuilderOptionsSetInlinerThreshold() function added to
LLVM 17's C API for this patch. Thanks! (Better than putting stuff
in llvmjit_wrap.c, if you can get it upstreamed in time.)

I thought I needed to block users from building with too-old clang and
too-new LLVM, but I haven't managed to find a combination that
actually breaks anything. I wouldn't recommend it, but for example
clang 10 bitcode seems to be inlinable without problems by LLVM 16 on
my system (I didn't use an assert build of LLVM though). I think that
could be a separate adjustment if we learn that we need to enforce or
document a constraint there.

So far I've tested LLVM versions 10, 15, 16, 17, 18 (= their main
branch) against PostgreSQL versions 14, 15, 16. I've attached the
versions that apply to master and 16, and pushed back-patches to 14
and 15 to public branches if anyone's interested[1]https://github.com/macdice/postgres/tree/llvm16-14 and -15. Back-patching
further seems a bit harder. I'm quite willing to do it, but ... do we
actually need to, ie does anyone really *require* old PostgreSQL
release branches to work with new LLVM?

(I'll start a separate thread about the related question of when we
get to drop support for old LLVMs.)

One point from an earlier email:

On Sat, Aug 12, 2023 at 6:09 AM Andres Freund <andres@anarazel.de> wrote:

AttributeTemplate(PG_FUNCTION_ARGS)
{
+     PGFunction      fp PG_USED_FOR_ASSERTS_ONLY;
+
+     fp = &AttributeTemplate;

Other parts of the file do this by putting the functions into
referenced_functions[], i'd copy that here and below.

Actually here I just wanted to assert that the 3 template functions
match certain function pointer types. To restate what these functions
are about: in the JIT code I need the function type, but we have only
the function pointer type, and it is now impossible to go from a
function pointer type to a function type, so I needed to define some
example functions with the right prototype (well, one of them existed
already but I needed more), and then I wanted to assert that they are
assignable to the appropriate function pointer types. Does that make
sense?

In this version I changed it to what I hope is a more obvious/explicit
expression of that goal:

+       AssertVariableIsOfType(&ExecEvalSubroutineTemplate,
+                              ExecEvalSubroutine);

[1]: https://github.com/macdice/postgres/tree/llvm16-14 and -15

Hi Thomas,

On Thu, 2023-09-21 at 08:22 +1200, Thomas Munro wrote:

So far I've tested LLVM versions 10, 15, 16, 17, 18 (= their main
branch) against PostgreSQL versions 14, 15, 16.  I've attached the
versions that apply to master and 16, and pushed back-patches to 14
and 15 to public branches if anyone's interested[1].  Back-patching
further seems a bit harder.  I'm quite willing to do it, but ... do we
actually need to, ie does anyone really *require* old PostgreSQL
release branches to work with new LLVM?

RHEL releases new LLVM version along with their new minor releases every
6 month, and we have to build older versions with new LLVM each time.
From RHEL point of view, it would be great if we can back-patch back to
v12 :(

Regards,
--
Devrim Gündüz
Open Source Solution Architect, PostgreSQL Major Contributor
Twitter: @DevrimGunduz , @DevrimGunduzTR

On Thu, Sep 21, 2023 at 12:24 PM Devrim Gündüz <devrim@gunduz.org> wrote:

On Thu, 2023-09-21 at 08:22 +1200, Thomas Munro wrote:

So far I've tested LLVM versions 10, 15, 16, 17, 18 (= their main
branch) against PostgreSQL versions 14, 15, 16. I've attached the
versions that apply to master and 16, and pushed back-patches to 14
and 15 to public branches if anyone's interested[1]. Back-patching
further seems a bit harder. I'm quite willing to do it, but ... do we
actually need to, ie does anyone really *require* old PostgreSQL
release branches to work with new LLVM?

RHEL releases new LLVM version along with their new minor releases every
6 month, and we have to build older versions with new LLVM each time.
From RHEL point of view, it would be great if we can back-patch back to
v12 :(

Got it. OK, I'll work on 12 and 13 now.

On Thu, Sep 21, 2023 at 12:47 PM Thomas Munro <thomas.munro@gmail.com> wrote:

On Thu, Sep 21, 2023 at 12:24 PM Devrim Gündüz <devrim@gunduz.org> wrote:

RHEL releases new LLVM version along with their new minor releases every
6 month, and we have to build older versions with new LLVM each time.
From RHEL point of view, it would be great if we can back-patch back to
v12 :(

Got it. OK, I'll work on 12 and 13 now.

The back-patch to 12 was a little trickier than anticipated, but after
taking a break and trying again I now have PG 12...17 patches that
I've tested against LLVM 10...18 (that's 54 combinations), in every
case only with the clang corresponding to LLVM.

For 12, I decided to back-patch the llvm_types_module variable that
was introduced in 13, to keep the code more similar.

For master, I had to rebase over Daniel's recent commits, which
required re-adding unused variables removed by 2dad308e, and
then changing a bunch of LLVM type constructors like LLVMInt8Type() to
the LLVMInt8TypeInContext(lc, ...) variants following the example of
9dce2203. Without that, type assertions in my LLVM 18 debug build
would fail (and maybe there could be a leak problem, though I'm not
sure that really applied to integer (non-struct) types).

I've attached only the patches for master, but the 12-16 versions are
available at https://github.com/macdice/postgres/tree/llvm16-$N in
case anyone has comments on those.

Le mercredi 11 octobre 2023, 10:59:50 CEST Thomas Munro a écrit :

The back-patch to 12 was a little trickier than anticipated, but after
taking a break and trying again I now have PG 12...17 patches that
I've tested against LLVM 10...18 (that's 54 combinations), in every
case only with the clang corresponding to LLVM.

Thank you Thomas for those patches, and the extensive testing, I will run my
own and let you know.

I've attached only the patches for master, but the 12-16 versions are
available at https://github.com/macdice/postgres/tree/llvm16-$N in
case anyone has comments on those.

For PG13 and PG12, it looks like the ExecEvalBoolSubroutineTemplate is not
used anywhere, as ExecEvalBoolSubroutine was introduced in PG14 if I'm not
mistaken.

Best regards,

--
Ronan Dunklau

Hi,

On 2023-10-11 21:59:50 +1300, Thomas Munro wrote:

+#else
+	LLVMPassBuilderOptionsRef options;
+	LLVMErrorRef err;
+	int			compile_optlevel;
+	char	   *passes;
+
+	if (context->base.flags & PGJIT_OPT3)
+		compile_optlevel = 3;
+	else
+		compile_optlevel = 0;
+
+	passes = psprintf("default<O%d>,mem2reg,function(no-op-function),no-op-module",
+					  compile_optlevel);

I don't think the "function(no-op-function),no-op-module" bit does something
particularly useful?

I also don't think we should add the mem2reg pass outside of -O0 - running it
after a real optimization pipeline doesn't seem useful and might even make the
code worse? mem2reg is included in default<O1> (and obviously also in O3).

Thanks for working on this stuff!

I'm working on setting up buildfarm animals for 16, 17, each once with
a normal and an assertion enabled LLVM build.

Greetings,

Andres Freund

On Thu, Oct 12, 2023 at 04:31:20PM -0700, Andres Freund wrote:
Hi,

On 2023-10-11 21:59:50 +1300, Thomas Munro wrote:

+#else
+	LLVMPassBuilderOptionsRef options;
+	LLVMErrorRef err;
+	int			compile_optlevel;
+	char	   *passes;
+
+	if (context->base.flags & PGJIT_OPT3)
+		compile_optlevel = 3;
+	else
+		compile_optlevel = 0;
+
+	passes = psprintf("default<O%d>,mem2reg,function(no-op-function),no-op-module",
+					  compile_optlevel);

I don't think the "function(no-op-function),no-op-module" bit does something
particularly useful?

Right, looks like leftovers after verifying which passes were actually
applied. My bad, could be removed.

I also don't think we should add the mem2reg pass outside of -O0 - running it
after a real optimization pipeline doesn't seem useful and might even make the
code worse? mem2reg is included in default<O1> (and obviously also in O3).

My understanding was that while mem2reg is included everywhere above
-O0, this set of passes won't hurt. But yeah, if you say it could
degrade the final result, it's better to not do this. I'll update this
part.

On Fri, Oct 13, 2023 at 11:06:21AM +0200, Dmitry Dolgov wrote:

On Thu, Oct 12, 2023 at 04:31:20PM -0700, Andres Freund wrote:
I don't think the "function(no-op-function),no-op-module" bit does something
particularly useful?

Right, looks like leftovers after verifying which passes were actually
applied. My bad, could be removed.

I also don't think we should add the mem2reg pass outside of -O0 - running it
after a real optimization pipeline doesn't seem useful and might even make the
code worse? mem2reg is included in default<O1> (and obviously also in O3).

My understanding was that while mem2reg is included everywhere above
-O0, this set of passes won't hurt. But yeah, if you say it could
degrade the final result, it's better to not do this. I'll update this
part.

Here is what I had in mind (only this part in the second patch was changed).

Hi,

On 2023-10-13 11:06:21 +0200, Dmitry Dolgov wrote:

On Thu, Oct 12, 2023 at 04:31:20PM -0700, Andres Freund wrote:
I also don't think we should add the mem2reg pass outside of -O0 - running it
after a real optimization pipeline doesn't seem useful and might even make the
code worse? mem2reg is included in default<O1> (and obviously also in O3).

My understanding was that while mem2reg is included everywhere above
-O0, this set of passes won't hurt. But yeah, if you say it could
degrade the final result, it's better to not do this. I'll update this
part.

It's indeed included anywhere above that, but adding it explicitly to the
schedule means it's excuted twice:

echo 'int foo(int a) { return a / 343; }' | clang-16 -emit-llvm -x c -c -o - -S -|sed -e 's/optnone//'|opt-17 -debug-pass-manager -passes='default<O1>,mem2reg' -o /dev/null 2>&1|grep Promote
Running pass: PromotePass on foo (2 instructions)
Running pass: PromotePass on foo (2 instructions)

The second one is in a point in the pipeline where it doesn't help. It also
requires another analysis pass to be executed unnecessarily.

Greetings,

Andres Freund

On 2023-10-13 16:44:13 +0200, Dmitry Dolgov wrote:

Here is what I had in mind (only this part in the second patch was changed).

Makes sense to me. I think we'll likely eventually want to use a custom
pipeline anyway, and I think we should consider using an optimization level
inbetween "not at all" "as hard as possible"...

Greetings,

Andres Freund

On Wed, Oct 11, 2023 at 10:31 PM Ronan Dunklau <ronan.dunklau@aiven.io> wrote:

Le mercredi 11 octobre 2023, 10:59:50 CEST Thomas Munro a écrit :

The back-patch to 12 was a little trickier than anticipated, but after
taking a break and trying again I now have PG 12...17 patches that
I've tested against LLVM 10...18 (that's 54 combinations), in every
case only with the clang corresponding to LLVM.

Thank you Thomas for those patches, and the extensive testing, I will run my
own and let you know.

Thanks! No news is good news, I hope? I'm hoping to commit this today.

I've attached only the patches for master, but the 12-16 versions are
available at https://github.com/macdice/postgres/tree/llvm16-$N in
case anyone has comments on those.

For PG13 and PG12, it looks like the ExecEvalBoolSubroutineTemplate is not
used anywhere, as ExecEvalBoolSubroutine was introduced in PG14 if I'm not
mistaken.

Right, looks like I can remove that in those branches.

On Sat, Oct 14, 2023 at 3:56 AM Andres Freund <andres@anarazel.de> wrote:

On 2023-10-13 16:44:13 +0200, Dmitry Dolgov wrote:

Here is what I had in mind (only this part in the second patch was changed).

Makes sense to me. I think we'll likely eventually want to use a custom
pipeline anyway, and I think we should consider using an optimization level
inbetween "not at all" "as hard as possible"...

Thanks Dmitry and Andres. I'm planning to commit these today if there
are no further comments.

Le vendredi 13 octobre 2023, 22:32:13 CEST Thomas Munro a écrit :

On Wed, Oct 11, 2023 at 10:31 PM Ronan Dunklau <ronan.dunklau@aiven.io>

wrote:

Le mercredi 11 octobre 2023, 10:59:50 CEST Thomas Munro a écrit :

The back-patch to 12 was a little trickier than anticipated, but after
taking a break and trying again I now have PG 12...17 patches that
I've tested against LLVM 10...18 (that's 54 combinations), in every
case only with the clang corresponding to LLVM.

Thank you Thomas for those patches, and the extensive testing, I will run
my own and let you know.

Thanks! No news is good news, I hope? I'm hoping to commit this today.

I've attached only the patches for master, but the 12-16 versions are
available at https://github.com/macdice/postgres/tree/llvm16-$N in
case anyone has comments on those.

For PG13 and PG12, it looks like the ExecEvalBoolSubroutineTemplate is not
used anywhere, as ExecEvalBoolSubroutine was introduced in PG14 if I'm not
mistaken.

Right, looks like I can remove that in those branches.

Oh sorry I thought I followed up. I ran the same stress testing involving
several hours of sqlsmith with all jit costs set to zero and didn't notice
anything with LLVM16.

Thank you !

--
Ronan Dunklau