Skip to content

[x86] Bug fix to CMP/Jcc optimisation that erroneously changed the condition

J. Gareth "Kit" Moreton requested to merge CuriousKit/optimisations:i39851 into main

Summary

This merge request fixes a jump-chain-shortcutting optimisation bug, first identified in #39851 (closed), where the first condition was changed to match the second condition, which is incorrect - once the first condition is determined to be a subset of (or equal to) the second condition, only the destination should be changed, not the actual condition.

Additionally, some reference count fixes were made in the same optimisation function (these are a separate commit).

System

  • Operating system: Microsoft Windows, almost certainly others
  • Processor architecture: i386, x86_64

What is the current bug behavior?

The following code causes an assertion failure because a conditional jump in the assembly language was changed from jl to jle to match the second jump (a variant of the code below is present in a commit that acts as a new regression test):

program test;

{$mode delphi}

{$OPTIMIZATION LEVEL1}
{$OPTIMIZATION PEEPHOLE}
{$C+}

function Fn1: Boolean;
begin
  Result := True;
end;

procedure TestCmpErr;
var
  I: Integer;
begin
  I := 0;
  if (I < 0) or (not Fn1()) then
  begin // this branch should NOT be executed
    ASSERT((I <= 0) and (not Fn1()));
    ExitCode := 1;
  end;
end;

begin
  TestCmpErr;
end.

What is the behavior after applying this patch?

The above program should compile and run successfully without ExitCode being set to 1.

Relevant logs and/or screenshots

The compiler and RTL sources received a lot of changes in the produced assembly language, showing that this bug is actually quite prevalent and may have been causing unexpected failures elsewhere. For example, in the bufdataset unit - before:

	...
.Lj293:
	cmpl	$0,-60(%rbp)
# Peephole Optimization: CMP/Jcc/@Lbl/CMP/Jcc -> CMP/Jcc, redirecting first jump
	jne	.Lj296
	cmpl	$0,-64(%rbp)
	jng	.Lj310
	...

After:

	...
.Lj293:
	cmpl	$0,-60(%rbp)
# Peephole Optimization: CMP/Jcc/@Lbl/CMP/Jcc -> CMP/Jcc, redirecting first jump
	jg	.Lj296
	cmpl	$0,-64(%rbp)
	jng	.Lj310
	...

Originally the jump was jg .Lj292, and the code at .Lj292 is as follows:

.Lj292:
	cmpl	$0,-60(%rbp)
	jne	.Lj296

Additional Notes

The corrected reference counts permit some new optimisations since some new dead labels have appeared. For example, in cfileutl - before:

	...
	testl	%eax,%eax
	jne	.Lj240
.Lj239:
	jmp	.Lj236
	.p2align 4,,10
	.p2align 3
.Lj240:
	movq	(%rbx),%rdx
	...

After:

	...
	testl	%eax,%eax
	je	.Lj236
	movq	(%rbx),%rdx
	...

Similarly in convutils - before:

	...
	cmpb	$0,34(%rcx,%rdx)
# Peephole Optimization: SETcc/TEST/Jcc -> SETcc/Jcc
	seteb	%dl
# Peephole Optimization: CMP/Jcc/@Lbl/CMP/Jcc -> CMP/Jcc, redirecting first jump
	jne	.Lj148
.Lj150:
	addl	$1,%edi
.Lj148:
	...

After:

	...
	cmpb	$0,34(%rcx,%rdx)
# Peephole Optimization: SETcc/TEST/Jcc -> SETcc/Jcc
	seteb	%dl
# Peephole Optimization: CMP/Jcc/@Lbl/CMP/Jcc -> CMP/Jcc, redirecting first jump
# Peephole Optimization: JccAdd2SetccAdd
	sete	%cl
	movzbl	%cl,%ecx
	addl	%ecx,%edi
.Lj148:
	...
Edited by J. Gareth "Kit" Moreton

Merge request reports