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.
1.
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=a7f107df2b700c859e4d9ad2ca66b07a465d6223
Andreas
From d63f681f7d0df06d493a1ec06a706f32e39e250e Mon Sep 17 00:00:00 2001
From: Andreas Karlsson <andr...@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
