RFR: 8312213: Remove unnecessary TEST instructions on x86 when flags reg will already be set

[Quan Anh Mai]

On Fri, 14 Jul 2023 09:10:24 GMT, Tobias Hotz <duke at openjdk.org> wrote:

>> This patch adds peephole rules to remove TEST instructions that operate on the result and right after a AND, XOR or OR instruction.
>> This pattern can emerge if the result of the and is compared against two values where one of the values is zero. The matcher does not have the capability to know that the instruction mentioned above also set the flag register to the same value.
>> According to https://www.felixcloutier.com/x86/and, https://www.felixcloutier.com/x86/xor, https://www.felixcloutier.com/x86/or and https://www.felixcloutier.com/x86/test the flags are set to same values for TEST, AND, XOR and OR, so this should be safe.
>> By adding peephole rules to remove the TEST instructions, the resulting assembly code can be shortend and a small speedup can be observed:
>> Results on Intel Core i5-8250U CPU
>> Before this patch:
>> 
>> Benchmark                                              Mode  Cnt    Score    Error  Units
>> TestRemovalPeephole.benchmarkAndTestFusableInt         avgt    8  182.353 ±  1.751  ns/op
>> TestRemovalPeephole.benchmarkAndTestFusableIntSingle   avgt    8    1.110 ±  0.002  ns/op
>> TestRemovalPeephole.benchmarkAndTestFusableLong        avgt    8  212.836 ±  0.310  ns/op
>> TestRemovalPeephole.benchmarkAndTestFusableLongSingle  avgt    8    2.072 ±  0.002  ns/op
>> TestRemovalPeephole.benchmarkOrTestFusableInt          avgt    8   72.052 ±  0.215  ns/op
>> TestRemovalPeephole.benchmarkOrTestFusableIntSingle    avgt    8    1.406 ±  0.002  ns/op
>> TestRemovalPeephole.benchmarkOrTestFusableLong         avgt    8  113.396 ±  0.666  ns/op
>> TestRemovalPeephole.benchmarkOrTestFusableLongSingle   avgt    8    1.183 ±  0.001  ns/op
>> TestRemovalPeephole.benchmarkXorTestFusableInt         avgt    8   88.683 ±  2.034  ns/op
>> TestRemovalPeephole.benchmarkXorTestFusableIntSingle   avgt    8    1.406 ±  0.002  ns/op
>> TestRemovalPeephole.benchmarkXorTestFusableLong        avgt    8  113.271 ±  0.602  ns/op
>> TestRemovalPeephole.benchmarkXorTestFusableLongSingle  avgt    8    1.183 ±  0.001  ns/op
>> 
>> After this patch:
>> 
>> Benchmark                                              Mode  Cnt    Score   Error  Units  Change
>> TestRemovalPeephole.benchmarkAndTestFusableInt         avgt    8  141.615 ± 4.747  ns/op  ~29% faster
>> TestRemovalPeephole.benchmarkAndTestFusableIntSingle   avgt    8    1.110 ± 0.002  ns/op  (unchanged)
>> TestRemovalPeephole.benchmarkAndTestFusableLong        avgt    8  213.249 ± 1.094  ns/op  (unchanged)
>> TestRemovalPeephole.bench...
>
> Thanks for the feedback!
> I agree that this is still far from a perfect solution and tracking the required flags would be ideal, but there many rules that use the flags register, and all of these would need to be tracked. If you are interested in that, feel free to take this PR as a starting point, but I'm currently not interested in pursuing that path. Otherwise we can still keep this PR in its current form IMO as a first step to removing redudant test instructions.

@ichttt Yes, I don't think tracking all the nodes that use flags is elegant, you can get the use from the test node, then iterate the operands of the use to find the `cmpOp` operand which you can know which flag it is using. For most nodes, this will be enough, and for `add`/`sub` the transformation can be applied if `cmpOp->ccode() == Assembler::zero || cmpOp->ccode() == Assembler::notZero`. I think try to do this for `add/sub` would be beneficial since they are more common and they can be macro-fuse with `jcc`.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/14172#issuecomment-1636001729