JIT: Remove some unnecessary instructions.
Hi hackers,
I find there are some unnecessary load/store instructions being
emitted by the JIT compiler.
E.g.,
```
v_boolnull = l_load(b, TypeStorageBool, v_resnullp, "");
v_boolvalue = l_load(b, TypeSizeT, v_resvaluep, "");
/* set resnull to boolnull */
LLVMBuildStore(b, v_boolnull, v_resnullp);
/* set revalue to boolvalue */
LLVMBuildStore(b, v_boolvalue, v_resvaluep);
```
v_boolnull is loaded from v_resnullp and stored to v_resnullp immediately.
v_boolvalue is loaded from v_resvaluep and stored to v_resvaluep immediately.
The attached patch is trying to fix them.
Best Regards,
Xing
Attachments:
0001-JIT-Remove-some-unnecessary-instructions.patchtext/x-patch; charset=US-ASCII; name=0001-JIT-Remove-some-unnecessary-instructions.patchDownload
From 6c7503b9e92c10b3dd9e6d33391273b854d53437 Mon Sep 17 00:00:00 2001
From: Xing Guo <higuoxing@gmail.com>
Date: Fri, 30 Aug 2024 11:43:45 +0800
Subject: [PATCH] JIT: Remove some unnecessary instructions.
There're some unnecessary instructions being emitted by the JIT
compiler. This patch removes them.
---
src/backend/jit/llvm/llvmjit_expr.c | 15 +--------------
1 file changed, 1 insertion(+), 14 deletions(-)
diff --git a/src/backend/jit/llvm/llvmjit_expr.c b/src/backend/jit/llvm/llvmjit_expr.c
index 48ccdb942a..1edc476c59 100644
--- a/src/backend/jit/llvm/llvmjit_expr.c
+++ b/src/backend/jit/llvm/llvmjit_expr.c
@@ -720,11 +720,6 @@ llvm_compile_expr(ExprState *state)
v_boolnull = l_load(b, TypeStorageBool, v_resnullp, "");
v_boolvalue = l_load(b, TypeSizeT, v_resvaluep, "");
- /* set resnull to boolnull */
- LLVMBuildStore(b, v_boolnull, v_resnullp);
- /* set revalue to boolvalue */
- LLVMBuildStore(b, v_boolvalue, v_resvaluep);
-
/* check if current input is NULL */
LLVMBuildCondBr(b,
LLVMBuildICmp(b, LLVMIntEQ, v_boolnull,
@@ -816,11 +811,6 @@ llvm_compile_expr(ExprState *state)
v_boolnull = l_load(b, TypeStorageBool, v_resnullp, "");
v_boolvalue = l_load(b, TypeSizeT, v_resvaluep, "");
- /* set resnull to boolnull */
- LLVMBuildStore(b, v_boolnull, v_resnullp);
- /* set revalue to boolvalue */
- LLVMBuildStore(b, v_boolvalue, v_resvaluep);
-
LLVMBuildCondBr(b,
LLVMBuildICmp(b, LLVMIntEQ, v_boolnull,
l_sbool_const(1), ""),
@@ -875,10 +865,8 @@ llvm_compile_expr(ExprState *state)
case EEOP_BOOL_NOT_STEP:
{
LLVMValueRef v_boolvalue;
- LLVMValueRef v_boolnull;
LLVMValueRef v_negbool;
- v_boolnull = l_load(b, TypeStorageBool, v_resnullp, "");
v_boolvalue = l_load(b, TypeSizeT, v_resvaluep, "");
v_negbool = LLVMBuildZExt(b,
@@ -887,8 +875,7 @@ llvm_compile_expr(ExprState *state)
l_sizet_const(0),
""),
TypeSizeT, "");
- /* set resnull to boolnull */
- LLVMBuildStore(b, v_boolnull, v_resnullp);
+
/* set revalue to !boolvalue */
LLVMBuildStore(b, v_negbool, v_resvaluep);
--
2.46.0
On 8/30/24 5:55 AM, Xing Guo wrote:
I find there are some unnecessary load/store instructions being
emitted by the JIT compiler.
Well spotted! All of these are obvious dead instructions and while LLVM
might be able to optimize them away there is no reason to create extra
work for the optimizer.
The patch looks good, applies and the tests passes.
Andreas
On Fri, Aug 30, 2024 at 8:50 PM Andreas Karlsson <andreas@proxel.se> wrote:
On 8/30/24 5:55 AM, Xing Guo wrote:
I find there are some unnecessary load/store instructions being
emitted by the JIT compiler.Well spotted! All of these are obvious dead instructions and while LLVM
might be able to optimize them away there is no reason to create extra
work for the optimizer.The patch looks good, applies and the tests passes.
Thanks for testing it! I spotted another unnecessary store instruction
and added it in my V2 patch.
Best Regards,
Xing
Attachments:
v2-0001-JIT-Remove-unnecessary-load-store-instructions.patchtext/x-patch; charset=US-ASCII; name=v2-0001-JIT-Remove-unnecessary-load-store-instructions.patchDownload
From ca30976ebf70ebed12ede7999d4ab637a9851641 Mon Sep 17 00:00:00 2001
From: Xing Guo <higuoxing@gmail.com>
Date: Mon, 2 Sep 2024 10:21:37 +0800
Subject: [PATCH v2] JIT: Remove unnecessary load/store instructions.
---
src/backend/jit/llvm/llvmjit_expr.c | 16 +---------------
1 file changed, 1 insertion(+), 15 deletions(-)
diff --git a/src/backend/jit/llvm/llvmjit_expr.c b/src/backend/jit/llvm/llvmjit_expr.c
index 48ccdb942a..7ece2e3d2e 100644
--- a/src/backend/jit/llvm/llvmjit_expr.c
+++ b/src/backend/jit/llvm/llvmjit_expr.c
@@ -720,11 +720,6 @@ llvm_compile_expr(ExprState *state)
v_boolnull = l_load(b, TypeStorageBool, v_resnullp, "");
v_boolvalue = l_load(b, TypeSizeT, v_resvaluep, "");
- /* set resnull to boolnull */
- LLVMBuildStore(b, v_boolnull, v_resnullp);
- /* set revalue to boolvalue */
- LLVMBuildStore(b, v_boolvalue, v_resvaluep);
-
/* check if current input is NULL */
LLVMBuildCondBr(b,
LLVMBuildICmp(b, LLVMIntEQ, v_boolnull,
@@ -816,11 +811,6 @@ llvm_compile_expr(ExprState *state)
v_boolnull = l_load(b, TypeStorageBool, v_resnullp, "");
v_boolvalue = l_load(b, TypeSizeT, v_resvaluep, "");
- /* set resnull to boolnull */
- LLVMBuildStore(b, v_boolnull, v_resnullp);
- /* set revalue to boolvalue */
- LLVMBuildStore(b, v_boolvalue, v_resvaluep);
-
LLVMBuildCondBr(b,
LLVMBuildICmp(b, LLVMIntEQ, v_boolnull,
l_sbool_const(1), ""),
@@ -875,10 +865,8 @@ llvm_compile_expr(ExprState *state)
case EEOP_BOOL_NOT_STEP:
{
LLVMValueRef v_boolvalue;
- LLVMValueRef v_boolnull;
LLVMValueRef v_negbool;
- v_boolnull = l_load(b, TypeStorageBool, v_resnullp, "");
v_boolvalue = l_load(b, TypeSizeT, v_resvaluep, "");
v_negbool = LLVMBuildZExt(b,
@@ -887,8 +875,7 @@ llvm_compile_expr(ExprState *state)
l_sizet_const(0),
""),
TypeSizeT, "");
- /* set resnull to boolnull */
- LLVMBuildStore(b, v_boolnull, v_resnullp);
+
/* set revalue to !boolvalue */
LLVMBuildStore(b, v_negbool, v_resvaluep);
@@ -1615,7 +1602,6 @@ llvm_compile_expr(ExprState *state)
LLVMPositionBuilderAtEnd(b, b_argsequal);
LLVMBuildStore(b, l_sbool_const(1), v_resnullp);
LLVMBuildStore(b, l_sizet_const(0), v_resvaluep);
- LLVMBuildStore(b, v_retval, v_resvaluep);
LLVMBuildBr(b, opblocks[opno + 1]);
break;
--
2.46.0
On 9/2/24 4:23 AM, Xing Guo wrote:
Thanks for testing it! I spotted another unnecessary store instruction
and added it in my V2 patch.
Another well-spotted unnecessary store. Nice!
I think this patch is ready for committer. It is simple and pretty
obviously correct.
Andreas
On 9/2/24 9:06 PM, Andreas Karlsson wrote:
On 9/2/24 4:23 AM, Xing Guo wrote:
Thanks for testing it! I spotted another unnecessary store instruction
and added it in my V2 patch.Another well-spotted unnecessary store. Nice!
I think this patch is ready for committer. It is simple and pretty
obviously correct.
Oh, and please add your patch to the commitfest app so it is not lost.
https://commitfest.postgresql.org/50/
Andreas
On 02/09/2024 22:06, Andreas Karlsson wrote:
On 9/2/24 4:23 AM, Xing Guo wrote:
Thanks for testing it! I spotted another unnecessary store instruction
and added it in my V2 patch.Another well-spotted unnecessary store. Nice!
I think this patch is ready for committer. It is simple and pretty
obviously correct.
Committed, thanks!
--
Heikki Linnakangas
Neon (https://neon.tech)