Re: broken JIT support on Fedora 40

From: Dmitry Dolgov <9erthalion6(at)gmail(dot)com>
To: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
Cc: Daniel Gustafsson <daniel(at)yesql(dot)se>, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: broken JIT support on Fedora 40
Date: 2024-04-09 10:05:21
Message-ID: 20240409100521.hphzzja6w3krlidx@erthalion.local
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On Tue, Apr 09, 2024 at 07:07:58PM +1200, Thomas Munro wrote:
> On Sat, Apr 6, 2024 at 5:01 AM Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote:
> > > > Yep, I think this is it. I've spent some hours trying to understand why
> > > > suddenly deform function has noundef ret attribute, when it shouldn't --
> > > > this explains it and the proposed change fixes the crash. One thing that
> > > > is still not clear to me though is why this copied attribute doesn't
> > > > show up in the bitcode dumped right before running inline pass (I've
> > > > added this to troubleshoot the issue).
> > >
> > > One thing to consider in this context is indeed adding "verify" pass as
> > > suggested in the PR, at least for the debugging configuration. Without the fix
> > > it immediately returns:
> > >
> > > Running analysis: VerifierAnalysis on deform_0_1
> > > Attribute 'noundef' applied to incompatible type!
> > >
> > > llvm error: Broken function found, compilation aborted!
> >
> > Here is what I have in mind. Interestingly enough, it also shows few
> > more errors besides "noundef":
> >
> > Intrinsic name not mangled correctly for type arguments! Should be: llvm.lifetime.end.p0
> > ptr @llvm.lifetime.end.p0i8
> >
> > It refers to the function from create_LifetimeEnd, not sure how
> > important is this.
>
> Would it be too slow to run the verify pass always, in assertion
> builds? Here's a patch for the original issue, and a patch to try
> that idea + a fix for that other complaint it spits out. The latter
> would only run for LLVM 17+, but that seems OK.

Sounds like a good idea. About the overhead, I've done a quick test on the
reproducer at hands, doing explain analyze in a tight loop and fetching
"optimization" timinigs. It gives quite visible difference 96ms p99 with verify
vs 46ms p99 without verify (and a rather low stddev, ~1.5ms). But I can
imagine it's acceptable for a build with assertions.

Btw, I've found there is a C-api for this exposed, which produces the same
warnings for me. Maybe it would be even better this way:

/**
* Toggle adding the VerifierPass for the PassBuilder, ensuring all functions
* inside the module is valid.
*/
void LLVMPassBuilderOptionsSetVerifyEach(LLVMPassBuilderOptionsRef Options,
LLVMBool VerifyEach);

+ /* In assertion builds, run the LLVM verify pass. */
+#ifdef USE_ASSERT_CHECKING
+ LLVMPassBuilderOptionsSetVerifyEach(options, true);
+#endif

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tomas Vondra 2024-04-09 10:18:32 Re: PostgreSQL 17 Release Management Team & Feature Freeze
Previous Message Richard Guo 2024-04-09 09:54:41 Incorrect handling of IS [NOT] NULL quals on inheritance parents