View Issue Details

IDProjectCategoryView StatusLast Update
0038339FPCCompilerpublic2021-01-10 11:24
ReporterJ. Gareth Moreton Assigned ToFlorian  
PrioritynormalSeverityminorReproducibilityN/A
Status resolvedResolutionfixed 
Platformi386 and x86_64OSMicrosoft Windows 
Product Version3.3.1 
Fixed in Version3.3.1 
Summary0038339: [Patch] MovzxCmp2CmpMovzx oversight
DescriptionAn apparent optimisation bug as listed in 0038334 is actually due to a different optimisation bug. The MovzxCmp2CmpMovzx optimisation didn't check to see if the cmp instruction actually used the same register as the movzx instruction. This patch corrects that.
Steps To ReproduceApply patch and confirm correct compilation and no regressions.
Additional InformationIn aoptx86.s before:

# [5806] if (reg in paramanager.get_volatile_registers_int(current_procinfo.procdef.proccalloption)) and
    movl U_$PROCINFO_$$_CURRENT_PROCINFO,%eax
    movl 24(%eax),%eax
    # Register dl allocated
    movb 85(%eax),%dl
    # Register eax released
    # Register eax allocated
    movl U_$PARAMGR_$$_PARAMANAGER,%eax
    # Register ecx allocated
    call CPUPARA$_$TCPUPARAMANAGER_$__$$_GET_VOLATILE_REGISTERS_INT$TPROCCALLOPTION$$TCPUREGISTERSET
    # Register ecx released
    movzwl %di,%edx
# Peephole Optimization: cmpl $15,%edx -> cmpw $15,%ax (smaller and minimises pipeline stall - MovzxCmp2CmpMovzx)
    cmpw $15,%ax
    movzwl %ax,%eax
    # Register eflags allocated
    jbe .Lj3446
    # Register eflags released
    clc
    jmp .Lj3447

After:

# [5806] if (reg in paramanager.get_volatile_registers_int(current_procinfo.procdef.proccalloption)) and
    movl U_$PROCINFO_$$_CURRENT_PROCINFO,%eax
    movl 24(%eax),%eax
    # Register dl allocated
    movb 85(%eax),%dl
    # Register eax released
    # Register eax allocated
    movl U_$PARAMGR_$$_PARAMANAGER,%eax
    # Register ecx allocated
    call CPUPARA$_$TCPUPARAMANAGER_$__$$_GET_VOLATILE_REGISTERS_INT$TPROCCALLOPTION$$TCPUREGISTERSET
    # Register ecx released
    movzwl %di,%edx
    movzwl %ax,%eax
    # Register eflags allocated
    cmpl $15,%edx
    jbe .Lj3446
    # Register eflags released
    clc
    jmp .Lj3447

----

This shows how useful the more complex comments are, because it shows that the instruction "cmpw $15,%ax" was originally "cmpl $15,%edx", which should instantly ring alarm bells because the register changed.

It was a complete coincidence that this fault only seemed to affect the JccAdd2SetccAdd optimisation (JccAdd2SetccAdd also affected a function in cutils, but this seemed to go undetected).
Tagsbugfix, compiler, i386, optimization, patch, x86_64
Fixed in Revision48124
FPCOldBugId
FPCTarget-
Attached Files

Relationships

related to 0038334 resolvedFlorian [Patch] [x86] JccAdd2SetccAdd optimisation is faulty 

Activities

J. Gareth Moreton

2021-01-10 00:28

developer  

MovzxCmp2CmpMovzx-fix.patch (913 bytes)   
Index: compiler/x86/aoptx86.pas
===================================================================
--- compiler/x86/aoptx86.pas	(revision 48117)
+++ compiler/x86/aoptx86.pas	(working copy)
@@ -7522,7 +7522,8 @@
                   (taicpu(hp1).opcode = A_TEST) and MatchOperand(taicpu(hp1).oper[0]^, taicpu(hp1).oper[1]^)
                 )
               ) and
-              (reg2opsize(taicpu(hp1).oper[1]^.reg) <= reg2opsize(taicpu(p).oper[1]^.reg)) then
+              (reg2opsize(taicpu(hp1).oper[1]^.reg) <= reg2opsize(taicpu(p).oper[1]^.reg)) and
+              SuperRegistersEqual(taicpu(p).oper[1]^.reg, taicpu(hp1).oper[1]^.reg) then
               begin
                 PreMessage := debug_op2str(taicpu(hp1).opcode) + debug_opsize2str(taicpu(hp1).opsize) + ' ' + debug_operstr(taicpu(hp1).oper[0]^) + ',' + debug_regname(taicpu(hp1).oper[1]^.reg) + ' -> ' + debug_op2str(taicpu(hp1).opcode);
 
MovzxCmp2CmpMovzx-fix.patch (913 bytes)   

Florian

2021-01-10 11:24

administrator   ~0128235

Thanks, applied.

Issue History

Date Modified Username Field Change
2021-01-10 00:28 J. Gareth Moreton New Issue
2021-01-10 00:28 J. Gareth Moreton File Added: MovzxCmp2CmpMovzx-fix.patch
2021-01-10 00:28 J. Gareth Moreton Relationship added related to 0038334
2021-01-10 04:02 J. Gareth Moreton Tag Attached: patch
2021-01-10 04:02 J. Gareth Moreton Tag Attached: compiler
2021-01-10 04:02 J. Gareth Moreton Tag Attached: optimization
2021-01-10 04:02 J. Gareth Moreton Tag Attached: i386
2021-01-10 04:02 J. Gareth Moreton Tag Attached: x86_64
2021-01-10 04:02 J. Gareth Moreton Tag Attached: bugfix
2021-01-10 11:24 Florian Assigned To => Florian
2021-01-10 11:24 Florian Status new => resolved
2021-01-10 11:24 Florian Resolution open => fixed
2021-01-10 11:24 Florian Fixed in Version => 3.3.1
2021-01-10 11:24 Florian Fixed in Revision => 48124
2021-01-10 11:24 Florian FPCTarget => -
2021-01-10 11:24 Florian Note Added: 0128235