Re: Don't codegen deform code for virtual tuples in expr eval for scan fetch

Hello,

In my previous patch 0001, the resulting opblock consisted of a single
br instruction to it's successor opblock. Such a block represents
unnecessary overhead. Even though such a block would be optimized
away, what if optimization is not performed (perhaps due to
jit_optimize_above_cost)? Perhaps we could be more aggressive. We
could maybe remove the opblock altogether. However, such a solution
is not without complexity.

Such unlinking is non-trivial, and is one of the things the
simplify-cfg pass takes care of. One solution could be to run this
pass ad-hoc for the evalexpr function. Or we could perform the
unlinking during codegen. Such a solution is presented below.

To unlink the current opblock from
the cfg we have to replace all of its uses. From the current state of
the code, these uses are either:
1. Terminator instruction of opblocks[i - 1]
2. Terminator instruction of some sub-op-block of opblocks[i - 1]. By
sub-op-block, I mean some block that is directly linked from opblock[i
- 1] but not opblocks[i].
3. Terminator instruction of the entry block.

We should replace all of these uses with opblocks[i + 1] and then and
only then can we delete opblocks[i]. My latest patch v2-0001 in my latest
patch set, achieves this.

I guard LLVMReplaceAllUsesWith() with Assert()s to ensure that we
don't accidentally replace non-terminator uses of opblocks[i], should
they be introduced in the future. If these asserts fail in the future,
replacing these non-terminator instructions with undefs constitutes a
commonly adopted solution. Otherwise, we can always fall back to making
opblocks[i] empty with just the unconditional br, as in my previous
patch 0001.

--
Soumyadeep

On Tue, Sep 17, 2019 at 11:54 PM Soumyadeep Chakraborty <
soumyadeep2007(at)gmail(dot)com> wrote:

> Hello Hackers,
>
> This is to address a TODO I found in the JIT expression evaluation
> code (opcode =
> EEOP_INNER_FETCHSOME/EEOP_OUTER_FETCHSOME/EEOP_SCAN_FETCHSOME):
>
> * TODO: skip nvalid check if slot is fixed and known to
> * be a virtual slot.
>
> Not only should we skip the nvalid check if the tuple is virtual, the
> whole basic block should be a no-op. There is no deforming to be done,
> JIT (slot_compile_deform) or otherwise (slot_getsomeattrs) for virtual
> tuples.
>
> Attached is a patch 0001 that achieves exactly that by:
>
> 1. Performing the check at the very beginning of the case.
> 2. Emitting an unconditional branch to the next opblock if we have a
> virtual tuple. We end up with a block with just the single
> unconditional branch instruction in this case. This block is optimized
> away when we run llvm_optimize_module().
>
> Also enclosed is another patch 0002 that adds on some minor
> refactoring of conditionals around whether to JIT deform or not.
>
> ---
> Soumyadeep Chakraborty
>