JIT: The nullness of casetest.value can be determined at the JIT compile time.
Hi hackers,
The nullness of casetest.value can be determined at the JIT compile
time. We can emit fewer codes by utilizing this property. The attached
patch is trying to fix it.
Best Regards,
Xing
Attachments:
v1-0001-JIT-The-nullness-of-casetest.value-can-be-determi.patchtext/x-patch; charset=US-ASCII; name=v1-0001-JIT-The-nullness-of-casetest.value-can-be-determi.patchDownload
From e366f50ec5fff46b537994c0ff4babe6646a97e5 Mon Sep 17 00:00:00 2001
From: Xing Guo <higuoxing@gmail.com>
Date: Sat, 31 Aug 2024 15:56:43 +0800
Subject: [PATCH v1] JIT: The nullness of casetest.value can be determined at
code emitting stage.
---
src/backend/jit/llvm/llvmjit_expr.c | 158 +++++++++++-----------------
1 file changed, 62 insertions(+), 96 deletions(-)
diff --git a/src/backend/jit/llvm/llvmjit_expr.c b/src/backend/jit/llvm/llvmjit_expr.c
index 48ccdb942a..0beef67e90 100644
--- a/src/backend/jit/llvm/llvmjit_expr.c
+++ b/src/backend/jit/llvm/llvmjit_expr.c
@@ -1202,53 +1202,37 @@ llvm_compile_expr(ExprState *state)
case EEOP_CASE_TESTVAL:
{
- LLVMBasicBlockRef b_avail,
- b_notavail;
- LLVMValueRef v_casevaluep,
- v_casevalue;
- LLVMValueRef v_casenullp,
- v_casenull;
- LLVMValueRef v_casevaluenull;
-
- b_avail = l_bb_before_v(opblocks[opno + 1],
- "op.%d.avail", opno);
- b_notavail = l_bb_before_v(opblocks[opno + 1],
- "op.%d.notavail", opno);
-
- v_casevaluep = l_ptr_const(op->d.casetest.value,
- l_ptr(TypeSizeT));
- v_casenullp = l_ptr_const(op->d.casetest.isnull,
- l_ptr(TypeStorageBool));
-
- v_casevaluenull =
- LLVMBuildICmp(b, LLVMIntEQ,
- LLVMBuildPtrToInt(b, v_casevaluep,
- TypeSizeT, ""),
- l_sizet_const(0), "");
- LLVMBuildCondBr(b, v_casevaluenull, b_notavail, b_avail);
-
- /* if casetest != NULL */
- LLVMPositionBuilderAtEnd(b, b_avail);
- v_casevalue = l_load(b, TypeSizeT, v_casevaluep, "");
- v_casenull = l_load(b, TypeStorageBool, v_casenullp, "");
- LLVMBuildStore(b, v_casevalue, v_resvaluep);
- LLVMBuildStore(b, v_casenull, v_resnullp);
- LLVMBuildBr(b, opblocks[opno + 1]);
+ LLVMValueRef v_casevalue, v_casenull;
- /* if casetest == NULL */
- LLVMPositionBuilderAtEnd(b, b_notavail);
- v_casevalue =
- l_load_struct_gep(b,
- StructExprContext,
- v_econtext,
- FIELDNO_EXPRCONTEXT_CASEDATUM, "");
- v_casenull =
- l_load_struct_gep(b,
- StructExprContext,
- v_econtext,
- FIELDNO_EXPRCONTEXT_CASENULL, "");
- LLVMBuildStore(b, v_casevalue, v_resvaluep);
- LLVMBuildStore(b, v_casenull, v_resnullp);
+ if (op->d.casetest.value)
+ {
+ LLVMValueRef v_casevaluep, v_casenullp;
+
+ v_casevaluep = l_ptr_const(op->d.casetest.value,
+ l_ptr(TypeSizeT));
+ v_casenullp = l_ptr_const(op->d.casetest.isnull,
+ l_ptr(TypeStorageBool));
+ v_casevalue = l_load(b, TypeSizeT, v_casevaluep, "");
+ v_casenull = l_load(b, TypeStorageBool, v_casenullp, "");
+
+ LLVMBuildStore(b, v_casevalue, v_resvaluep);
+ LLVMBuildStore(b, v_casenull, v_resnullp);
+ }
+ else
+ {
+ v_casevalue =
+ l_load_struct_gep(b,
+ StructExprContext,
+ v_econtext,
+ FIELDNO_EXPRCONTEXT_CASEDATUM, "");
+ v_casenull =
+ l_load_struct_gep(b,
+ StructExprContext,
+ v_econtext,
+ FIELDNO_EXPRCONTEXT_CASENULL, "");
+ LLVMBuildStore(b, v_casevalue, v_resvaluep);
+ LLVMBuildStore(b, v_casenull, v_resnullp);
+ }
LLVMBuildBr(b, opblocks[opno + 1]);
break;
@@ -1832,58 +1816,40 @@ llvm_compile_expr(ExprState *state)
case EEOP_DOMAIN_TESTVAL:
{
- LLVMBasicBlockRef b_avail,
- b_notavail;
- LLVMValueRef v_casevaluep,
- v_casevalue;
- LLVMValueRef v_casenullp,
- v_casenull;
- LLVMValueRef v_casevaluenull;
-
- b_avail = l_bb_before_v(opblocks[opno + 1],
- "op.%d.avail", opno);
- b_notavail = l_bb_before_v(opblocks[opno + 1],
- "op.%d.notavail", opno);
-
- v_casevaluep = l_ptr_const(op->d.casetest.value,
- l_ptr(TypeSizeT));
- v_casenullp = l_ptr_const(op->d.casetest.isnull,
- l_ptr(TypeStorageBool));
-
- v_casevaluenull =
- LLVMBuildICmp(b, LLVMIntEQ,
- LLVMBuildPtrToInt(b, v_casevaluep,
- TypeSizeT, ""),
- l_sizet_const(0), "");
- LLVMBuildCondBr(b,
- v_casevaluenull,
- b_notavail, b_avail);
-
- /* if casetest != NULL */
- LLVMPositionBuilderAtEnd(b, b_avail);
- v_casevalue = l_load(b, TypeSizeT, v_casevaluep, "");
- v_casenull = l_load(b, TypeStorageBool, v_casenullp, "");
- LLVMBuildStore(b, v_casevalue, v_resvaluep);
- LLVMBuildStore(b, v_casenull, v_resnullp);
- LLVMBuildBr(b, opblocks[opno + 1]);
+ LLVMValueRef v_casevalue, v_casenull;
- /* if casetest == NULL */
- LLVMPositionBuilderAtEnd(b, b_notavail);
- v_casevalue =
- l_load_struct_gep(b,
- StructExprContext,
- v_econtext,
- FIELDNO_EXPRCONTEXT_DOMAINDATUM,
- "");
- v_casenull =
- l_load_struct_gep(b,
- StructExprContext,
- v_econtext,
- FIELDNO_EXPRCONTEXT_DOMAINNULL,
- "");
- LLVMBuildStore(b, v_casevalue, v_resvaluep);
- LLVMBuildStore(b, v_casenull, v_resnullp);
+ if (op->d.casetest.value)
+ {
+ LLVMValueRef v_casevaluep, v_casenullp;
+ v_casevaluep = l_ptr_const(op->d.casetest.value,
+ l_ptr(TypeSizeT));
+ v_casenullp = l_ptr_const(op->d.casetest.isnull,
+ l_ptr(TypeStorageBool));
+
+ v_casevalue = l_load(b, TypeSizeT, v_casevaluep, "");
+ v_casenull = l_load(b, TypeStorageBool, v_casenullp, "");
+
+ LLVMBuildStore(b, v_casevalue, v_resvaluep);
+ LLVMBuildStore(b, v_casenull, v_resnullp);
+ }
+ else
+ {
+ v_casevalue =
+ l_load_struct_gep(b,
+ StructExprContext,
+ v_econtext,
+ FIELDNO_EXPRCONTEXT_DOMAINDATUM,
+ "");
+ v_casenull =
+ l_load_struct_gep(b,
+ StructExprContext,
+ v_econtext,
+ FIELDNO_EXPRCONTEXT_DOMAINNULL,
+ "");
+ LLVMBuildStore(b, v_casevalue, v_resvaluep);
+ LLVMBuildStore(b, v_casenull, v_resnullp);
+ }
LLVMBuildBr(b, opblocks[opno + 1]);
break;
}
--
2.46.0
On 8/31/24 10:04 AM, Xing Guo wrote:
The nullness of casetest.value can be determined at the JIT compile
time. We can emit fewer codes by utilizing this property. The attached
patch is trying to fix it.
I have not reviewed the code yet but the idea seems good.
But I wonder if we shouldn't instead simplify the code a bit by
specializing these steps when generating them instead of doing the work
runtime/while generating machine code. Yes, I doubt the performance
benefits matter but I personally think the code is cleaner before my
patch than after it.
Long term it would be nice to get rid off
caseValue_datum/domainValue_datum as mentioned by Andres[1] but that is
a bigger job so think that either your patch or my patch would make
sense to apply before that.
Andreas
Attachments:
v1-0001-Specialize-EEOP_-_TESTVAL-steps.patchtext/x-patch; charset=UTF-8; name=v1-0001-Specialize-EEOP_-_TESTVAL-steps.patchDownload
From d63f681f7d0df06d493a1ec06a706f32e39e250e Mon Sep 17 00:00:00 2001
From: Andreas Karlsson <andreas@proxel.se>
Date: Tue, 3 Sep 2024 13:53:21 +0200
Subject: [PATCH v1] Specialize EEOP_*_TESTVAL steps
Refactor the EEOP_CASE_TESTVAL and EEOP_DOMAIN_TESTVAL steps by
deciding if we should read from caseValue_datum/domainValue_datum
or not when generating the executor steps rather than doing so at
runtime.
This gives a minor performance benefit but the real goal is to make
the code less surprising and easier to follow.
---
src/backend/executor/execExpr.c | 27 +++++++----
src/backend/executor/execExprInterp.c | 49 +++++++------------
src/backend/jit/llvm/llvmjit_expr.c | 68 +++++----------------------
src/include/executor/execExpr.h | 2 +
4 files changed, 48 insertions(+), 98 deletions(-)
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 63289ee35e..aaa67d3580 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -1844,11 +1844,15 @@ ExecInitExprRec(Expr *node, ExprState *state,
* That can happen because some parts of the system abuse
* CaseTestExpr to cause a read of a value externally supplied
* in econtext->caseValue_datum. We'll take care of that
- * scenario at runtime.
+ * by generating a specialized operation.
*/
- scratch.opcode = EEOP_CASE_TESTVAL;
- scratch.d.casetest.value = state->innermost_caseval;
- scratch.d.casetest.isnull = state->innermost_casenull;
+ if (state->innermost_caseval == NULL)
+ scratch.opcode = EEOP_CASE_TESTVAL_EXT;
+ else {
+ scratch.opcode = EEOP_CASE_TESTVAL;
+ scratch.d.casetest.value = state->innermost_caseval;
+ scratch.d.casetest.isnull = state->innermost_casenull;
+ }
ExprEvalPushStep(state, &scratch);
break;
@@ -2535,12 +2539,17 @@ ExecInitExprRec(Expr *node, ExprState *state,
* a standalone domain check rather than one embedded in a
* larger expression. In that case we must read from
* econtext->domainValue_datum. We'll take care of that
- * scenario at runtime.
+ * by generating a specialized step.
*/
- scratch.opcode = EEOP_DOMAIN_TESTVAL;
- /* we share instruction union variant with case testval */
- scratch.d.casetest.value = state->innermost_domainval;
- scratch.d.casetest.isnull = state->innermost_domainnull;
+ if (state->innermost_domainval == NULL)
+ scratch.opcode = EEOP_DOMAIN_TESTVAL_EXT;
+ else
+ {
+ scratch.opcode = EEOP_DOMAIN_TESTVAL;
+ /* we share instruction union variant with case testval */
+ scratch.d.casetest.value = state->innermost_domainval;
+ scratch.d.casetest.isnull = state->innermost_domainnull;
+ }
ExprEvalPushStep(state, &scratch);
break;
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index a6c47f61e0..83c8ae88dd 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -452,6 +452,7 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
&&CASE_EEOP_PARAM_CALLBACK,
&&CASE_EEOP_PARAM_SET,
&&CASE_EEOP_CASE_TESTVAL,
+ &&CASE_EEOP_CASE_TESTVAL_EXT,
&&CASE_EEOP_MAKE_READONLY,
&&CASE_EEOP_IOCOERCE,
&&CASE_EEOP_IOCOERCE_SAFE,
@@ -475,6 +476,7 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
&&CASE_EEOP_SBSREF_ASSIGN,
&&CASE_EEOP_SBSREF_FETCH,
&&CASE_EEOP_DOMAIN_TESTVAL,
+ &&CASE_EEOP_DOMAIN_TESTVAL_EXT,
&&CASE_EEOP_DOMAIN_NOTNULL,
&&CASE_EEOP_DOMAIN_CHECK,
&&CASE_EEOP_HASHDATUM_SET_INITVAL,
@@ -1107,45 +1109,26 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
}
EEO_CASE(EEOP_CASE_TESTVAL)
+ EEO_CASE(EEOP_DOMAIN_TESTVAL)
{
- /*
- * Normally upper parts of the expression tree have setup the
- * values to be returned here, but some parts of the system
- * currently misuse {caseValue,domainValue}_{datum,isNull} to set
- * run-time data. So if no values have been set-up, use
- * ExprContext's. This isn't pretty, but also not *that* ugly,
- * and this is unlikely to be performance sensitive enough to
- * worry about an extra branch.
- */
- if (op->d.casetest.value)
- {
- *op->resvalue = *op->d.casetest.value;
- *op->resnull = *op->d.casetest.isnull;
- }
- else
- {
- *op->resvalue = econtext->caseValue_datum;
- *op->resnull = econtext->caseValue_isNull;
- }
+ *op->resvalue = *op->d.casetest.value;
+ *op->resnull = *op->d.casetest.isnull;
EEO_NEXT();
}
- EEO_CASE(EEOP_DOMAIN_TESTVAL)
+ EEO_CASE(EEOP_CASE_TESTVAL_EXT)
{
- /*
- * See EEOP_CASE_TESTVAL comment.
- */
- if (op->d.casetest.value)
- {
- *op->resvalue = *op->d.casetest.value;
- *op->resnull = *op->d.casetest.isnull;
- }
- else
- {
- *op->resvalue = econtext->domainValue_datum;
- *op->resnull = econtext->domainValue_isNull;
- }
+ *op->resvalue = econtext->caseValue_datum;
+ *op->resnull = econtext->caseValue_isNull;
+
+ EEO_NEXT();
+ }
+
+ EEO_CASE(EEOP_DOMAIN_TESTVAL_EXT)
+ {
+ *op->resvalue = econtext->domainValue_datum;
+ *op->resnull = econtext->domainValue_isNull;
EEO_NEXT();
}
diff --git a/src/backend/jit/llvm/llvmjit_expr.c b/src/backend/jit/llvm/llvmjit_expr.c
index 48ccdb942a..b0780188bd 100644
--- a/src/backend/jit/llvm/llvmjit_expr.c
+++ b/src/backend/jit/llvm/llvmjit_expr.c
@@ -1201,42 +1201,32 @@ llvm_compile_expr(ExprState *state)
}
case EEOP_CASE_TESTVAL:
+ case EEOP_DOMAIN_TESTVAL:
{
- LLVMBasicBlockRef b_avail,
- b_notavail;
LLVMValueRef v_casevaluep,
v_casevalue;
LLVMValueRef v_casenullp,
v_casenull;
- LLVMValueRef v_casevaluenull;
-
- b_avail = l_bb_before_v(opblocks[opno + 1],
- "op.%d.avail", opno);
- b_notavail = l_bb_before_v(opblocks[opno + 1],
- "op.%d.notavail", opno);
v_casevaluep = l_ptr_const(op->d.casetest.value,
l_ptr(TypeSizeT));
v_casenullp = l_ptr_const(op->d.casetest.isnull,
l_ptr(TypeStorageBool));
- v_casevaluenull =
- LLVMBuildICmp(b, LLVMIntEQ,
- LLVMBuildPtrToInt(b, v_casevaluep,
- TypeSizeT, ""),
- l_sizet_const(0), "");
- LLVMBuildCondBr(b, v_casevaluenull, b_notavail, b_avail);
-
- /* if casetest != NULL */
- LLVMPositionBuilderAtEnd(b, b_avail);
v_casevalue = l_load(b, TypeSizeT, v_casevaluep, "");
v_casenull = l_load(b, TypeStorageBool, v_casenullp, "");
LLVMBuildStore(b, v_casevalue, v_resvaluep);
LLVMBuildStore(b, v_casenull, v_resnullp);
+
LLVMBuildBr(b, opblocks[opno + 1]);
+ break;
+ }
+
+ case EEOP_CASE_TESTVAL_EXT:
+ {
+ LLVMValueRef v_casevalue;
+ LLVMValueRef v_casenull;
- /* if casetest == NULL */
- LLVMPositionBuilderAtEnd(b, b_notavail);
v_casevalue =
l_load_struct_gep(b,
StructExprContext,
@@ -1830,45 +1820,11 @@ llvm_compile_expr(ExprState *state)
LLVMBuildBr(b, opblocks[opno + 1]);
break;
- case EEOP_DOMAIN_TESTVAL:
+ case EEOP_DOMAIN_TESTVAL_EXT:
{
- LLVMBasicBlockRef b_avail,
- b_notavail;
- LLVMValueRef v_casevaluep,
- v_casevalue;
- LLVMValueRef v_casenullp,
- v_casenull;
- LLVMValueRef v_casevaluenull;
-
- b_avail = l_bb_before_v(opblocks[opno + 1],
- "op.%d.avail", opno);
- b_notavail = l_bb_before_v(opblocks[opno + 1],
- "op.%d.notavail", opno);
-
- v_casevaluep = l_ptr_const(op->d.casetest.value,
- l_ptr(TypeSizeT));
- v_casenullp = l_ptr_const(op->d.casetest.isnull,
- l_ptr(TypeStorageBool));
-
- v_casevaluenull =
- LLVMBuildICmp(b, LLVMIntEQ,
- LLVMBuildPtrToInt(b, v_casevaluep,
- TypeSizeT, ""),
- l_sizet_const(0), "");
- LLVMBuildCondBr(b,
- v_casevaluenull,
- b_notavail, b_avail);
-
- /* if casetest != NULL */
- LLVMPositionBuilderAtEnd(b, b_avail);
- v_casevalue = l_load(b, TypeSizeT, v_casevaluep, "");
- v_casenull = l_load(b, TypeStorageBool, v_casenullp, "");
- LLVMBuildStore(b, v_casevalue, v_resvaluep);
- LLVMBuildStore(b, v_casenull, v_resnullp);
- LLVMBuildBr(b, opblocks[opno + 1]);
+ LLVMValueRef v_casevalue;
+ LLVMValueRef v_casenull;
- /* if casetest == NULL */
- LLVMPositionBuilderAtEnd(b, b_notavail);
v_casevalue =
l_load_struct_gep(b,
StructExprContext,
diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h
index eec0aa699e..4146aad88d 100644
--- a/src/include/executor/execExpr.h
+++ b/src/include/executor/execExpr.h
@@ -165,6 +165,7 @@ typedef enum ExprEvalOp
/* return CaseTestExpr value */
EEOP_CASE_TESTVAL,
+ EEOP_CASE_TESTVAL_EXT,
/* apply MakeExpandedObjectReadOnly() to target value */
EEOP_MAKE_READONLY,
@@ -228,6 +229,7 @@ typedef enum ExprEvalOp
/* evaluate value for CoerceToDomainValue */
EEOP_DOMAIN_TESTVAL,
+ EEOP_DOMAIN_TESTVAL_EXT,
/* evaluate a domain's NOT NULL constraint */
EEOP_DOMAIN_NOTNULL,
--
2.43.0
On Tue, Sep 3, 2024 at 8:09 PM Andreas Karlsson <andreas@proxel.se> wrote:
On 8/31/24 10:04 AM, Xing Guo wrote:
The nullness of casetest.value can be determined at the JIT compile
time. We can emit fewer codes by utilizing this property. The attached
patch is trying to fix it.I have not reviewed the code yet but the idea seems good.
But I wonder if we shouldn't instead simplify the code a bit by
specializing these steps when generating them instead of doing the work
runtime/while generating machine code. Yes, I doubt the performance
benefits matter but I personally think the code is cleaner before my
patch than after it.
+1 to the idea.
Long term it would be nice to get rid off
caseValue_datum/domainValue_datum as mentioned by Andres[1] but that is
a bigger job so think that either your patch or my patch would make
sense to apply before that.
I think your patch makes more sense than mine! Thanks!
Best Regards,
Xing
On 03/09/2024 18:18, Xing Guo wrote:
On Tue, Sep 3, 2024 at 8:09 PM Andreas Karlsson <andreas@proxel.se> wrote:
On 8/31/24 10:04 AM, Xing Guo wrote:
The nullness of casetest.value can be determined at the JIT compile
time. We can emit fewer codes by utilizing this property. The attached
patch is trying to fix it.I have not reviewed the code yet but the idea seems good.
But I wonder if we shouldn't instead simplify the code a bit by
specializing these steps when generating them instead of doing the work
runtime/while generating machine code. Yes, I doubt the performance
benefits matter but I personally think the code is cleaner before my
patch than after it.+1 to the idea.
Long term it would be nice to get rid off
caseValue_datum/domainValue_datum as mentioned by Andres[1] but that is
a bigger job so think that either your patch or my patch would make
sense to apply before that.I think your patch makes more sense than mine! Thanks!
Huge +1 for cleaning up this abuse of caseValue_datum/domainValue_datum.
While correct and sensible if we continue the abuse, these patches feel
like putting lipstick on a pig.
I think the most straightforward way to clean that up is to replace
caseValue and domainValue in ExprContext with a generic array of
"expression params", and a new Expr node type to refer to them.
Basically, the same as we have now, but with explicit names to indicate
that the values are supplied externally.
In
/messages/by-id/20230302200549.l2ikytmnqzvy5a7a@alap3.anarazel.de,
Andres hinted at using ParamExecData for these. That'd be nice, but
seems harder. Those params are reserved at planning time, so we'd need
to somehow allocate more of them in ExecInitExpr, or move the
bookkeeping to the planner. But just basically renaming "caseValue" to
"externalValue" seems straightforward.
PARAM_EXECs are also a slightly more expensive to evaluate, because they
support lazy evaluation of the subplan. This goes off-topic, but I
wonder if there are cases where we could know at ExecInitExpr() time
that the parameter must be already evaluated when it's referred, and
skip the lazy checks?
--
Heikki Linnakangas
Neon (https://neon.tech)
Heikki Linnakangas <hlinnaka@iki.fi> writes:
Huge +1 for cleaning up this abuse of caseValue_datum/domainValue_datum.
While correct and sensible if we continue the abuse, these patches feel
like putting lipstick on a pig.
Agreed. I spent some time trying to do it better, with results
shown at [1]/messages/by-id/3068812.1738206654@sss.pgh.pa.us. If we adopt that idea, then the executor's support
for CaseTestExpr will go away, so there's little point in pursuing
that half of the patch given here. However, I concluded that there's
insufficient reason to redesign CoerceToDomainValue, so we could
still push forward with that half of this patch.
regards, tom lane
I wrote:
Agreed. I spent some time trying to do it better, with results
shown at [1]. If we adopt that idea, then the executor's support
for CaseTestExpr will go away, so there's little point in pursuing
that half of the patch given here. However, I concluded that there's
insufficient reason to redesign CoerceToDomainValue, so we could
still push forward with that half of this patch.
On further consideration, we might as well do this whole patch.
The patch I'm proposing over there is large and might well not
land in v18 (or ever), so let's get this low-hanging fruit.
I pushed Andreas's patch with some cosmetic tidying, mainly
that I felt it better not to try to combine duplicate-ish
code for the EEOP_CASE_TESTVAL and EEOP_DOMAIN_TESTVAL cases.
We're after understandability here after all, and it's still
a net code savings even with that duplication.
regards, tom lane