View Issue Details

IDProjectCategoryView StatusLast Update
0033909FPCCompilerpublic2019-11-09 21:26
ReporterDo-wan KimAssigned ToFlorian 
PriorityurgentSeveritycrashReproducibilityalways
Status closedResolutionfixed 
Platformx86OSWindowsOS Version10
Product Version3.1.1Product Build39307 
Target Version3.1.1Fixed in Version3.1.1 
Summary0033909: New Jcc optimization makes error
DescriptionNew Jcc optimization makes error on cross compiling x86-64.

Revert to 39306, it works ok.
Additional InformationFree Pascal Compiler version 3.1.1-r39307 [2018/06/26] for x86_64
Copyright (c) 1993-2018 by Florian Klaempfl and others
(1002) Target OS: Win64 for x64
(3104) Compiling lazutils.pas
(3104) Compiling ttdebug.pas
C:\development\lazarus\components\lazutils\ttdebug.pas(653,8) Error: (1026) Compilation raised exception internally
C:\development\lazarus\components\lazutils\ttdebug.pas(653,8) Fatal: (1018) Compilation aborted
An unhandled exception occurred at $08D8C011:
EAccessViolation: Access violation
  $08D8C011

Error: C:\development\fpc\bin\i386-win32\ppcrossx64.exe returned an error exitcode
Error: (lazarus) Compile package LazUtils 1.0: stopped with exit code 1
Error: (lazbuild) LazUtils 1.0 compilation failed
TagsNo tags attached.
Fixed in Revision39353
FPCOldBugId
FPCTarget
Attached Files
  • fix_39307_aoptx86.pas.patch (742 bytes)
    Index: compiler/x86/aoptx86.pas
    ===================================================================
    --- compiler/x86/aoptx86.pas	(revision 39307)
    +++ compiler/x86/aoptx86.pas	(working copy)
    @@ -2284,7 +2284,7 @@
     
                 taicpu(hp2).SetCondition(SetC);
     
    -            if not RegUsedAfterInstruction(taicpu(p).oper[0]^.reg, hp2, TmpUsedRegs) then
    +            (* if not RegUsedAfterInstruction(taicpu(p).oper[0]^.reg, hp2, TmpUsedRegs) then
                   begin
                     asml.Remove(p);
                     UpdateUsedRegs(next);
    @@ -2291,7 +2291,7 @@
                     p.Free;
                     Result := True;
                     p := hp2;
    -              end;
    +              end; *)
     
                 ReleaseUsedRegs(TmpUsedRegs);
     
    
  • fix-corss64error-aoptx86.pas.patch (926 bytes)
    Index: compiler/x86/aoptx86.pas
    ===================================================================
    --- compiler/x86/aoptx86.pas	(revision 39311)
    +++ compiler/x86/aoptx86.pas	(working copy)
    @@ -2686,7 +2686,7 @@
                       end;
                   end;
     
    -            if (hp1.typ=ait_label) and (TAsmLabel(taicpu(p).oper[0]^.ref^.symbol) = tai_label(hp1).labsym) then
    +            (* if (hp1.typ=ait_label) and (TAsmLabel(taicpu(p).oper[0]^.ref^.symbol) = tai_label(hp1).labsym) then
                   begin
                     { If Jcc is immediately followed by the label that it's supposed to jump to, remove it }
                     UpdateUsedRegs(tai(p.next));
    @@ -2698,7 +2698,7 @@
                     Result := True;
                     p := hp1;
                     Exit;
    -              end;
    +              end; *)
               end;
     {$ifndef i8086}
             if CPUX86_HAS_CMOV in cpu_capabilities[current_settings.cputype] then
    
  • i33909-recreate.patch (16,726 bytes)
    Index: compiler/aoptobj.pas
    ===================================================================
    --- compiler/aoptobj.pas	(revision 39346)
    +++ compiler/aoptobj.pas	(working copy)
    @@ -1671,7 +1671,6 @@
             ClearUsedRegs;
             while (p <> BlockEnd) Do
               begin
    -            UpdateUsedRegs(tai(p.next));
                 if PeepHoleOptPass2Cpu(p) then
                   continue;
                 if assigned(p) then
    Index: compiler/ogbase.pas
    ===================================================================
    --- compiler/ogbase.pas	(revision 39346)
    +++ compiler/ogbase.pas	(working copy)
    @@ -1351,6 +1351,9 @@
           begin
             if assigned(asmsym) then
               begin
    +            if asmsym.typ = AT_NONE then
    +              InternalError(2018062800);
    +
                 if not assigned(asmsym.cachedObjSymbol) then
                   begin
                     result:=symboldefine(asmsym.name,asmsym.bind,asmsym.typ);
    Index: compiler/x86/aoptx86.pas
    ===================================================================
    --- compiler/x86/aoptx86.pas	(revision 39346)
    +++ compiler/x86/aoptx86.pas	(working copy)
    @@ -2240,6 +2240,7 @@
             hp1,hp2,next: tai; SetC, JumpC: TAsmCond;
           begin
             Result:=false;
    +
             if MatchOpType(taicpu(p),top_reg) and
               GetNextInstruction(p, hp1) and
               MatchInstruction(hp1, A_TEST, [S_B]) and
    @@ -2622,26 +2623,29 @@
     
         function TX86AsmOptimizer.OptPass2Jcc(var p : tai) : boolean;
           var
    -        hp1,hp2,hp3: tai;
    +        hp1,hp2,hp3,hp4,hpmov2: tai;
             carryadd_opcode : TAsmOp;
             l : Longint;
             condition : TAsmCond;
    +        symbol: TAsmSymbol;
           begin
             result:=false;
             if GetNextInstruction(p,hp1) then
               begin
    +            symbol := TAsmLabel(taicpu(p).oper[0]^.ref^.symbol);
    +
                 if (hp1.typ=ait_instruction) and
                    GetNextInstruction(hp1,hp2) and (hp2.typ=ait_label) and
    -               (Tasmlabel(Taicpu(p).oper[0]^.ref^.symbol)=Tai_label(hp2).labsym) then
    +               (Tasmlabel(symbol) = Tai_label(hp2).labsym) then
                      { jb @@1                            cmc
                        inc/dec operand           -->     adc/sbb operand,0
    -         	@@1:
    +                   @@1:
     
    -         	... and ...
    +                   ... and ...
     
                        jnb @@1
                        inc/dec operand           -->     adc/sbb operand,0
    -         	@@1: }
    +                   @@1: }
                   begin
                     carryadd_opcode:=A_NONE;
                     if Taicpu(p).condition in [C_NAE,C_B] then
    @@ -2686,20 +2690,49 @@
                       end;
                   end;
     
    -            if (hp1.typ=ait_label) and (TAsmLabel(taicpu(p).oper[0]^.ref^.symbol) = tai_label(hp1).labsym) then
    +            if ((hp1.typ = ait_label) and (symbol = tai_label(hp1).labsym))
    +                or ((hp1.typ = ait_align) and GetNextInstruction(hp1, hp2) and (hp2.typ = ait_label) and (symbol = tai_label(hp2).labsym)) then
                   begin
                     { If Jcc is immediately followed by the label that it's supposed to jump to, remove it }
    -                UpdateUsedRegs(tai(p.next));
    -
                     DebugMsg(SPeepholeOptimization + 'Removed conditional jump whose destination was immediately after it', p);
    +                UpdateUsedRegs(hp1);
     
    +                TAsmLabel(symbol).decrefs;
    +                { if the label refs. reach zero, remove any alignment before the label }
    +                if (hp1.typ = ait_align) then
    +                  begin
    +                    UpdateUsedRegs(hp2);
    +                    if (TAsmLabel(symbol).getrefs = 0) then
    +                    begin
    +                      asml.Remove(hp1);
    +                      hp1.Free;
    +                    end;
    +                    hp1 := hp2; { Set hp1 to the label }
    +                  end;
    +
                     asml.remove(p);
                     p.free;
    -                Result := True;
    -                p := hp1;
    +
    +                if (TAsmLabel(symbol).getrefs = 0) then
    +                  begin
    +                    GetNextInstruction(hp1, p); { Instruction following the label }
    +                    asml.remove(hp1);
    +                    hp1.free;
    +
    +                    UpdateUsedRegs(p);
    +                    Result := True;
    +                  end
    +                else
    +                  begin
    +                    { We don't need to set the result to True because we know hp1
    +                      is a label and won't trigger any optimisation routines. [Kit] }
    +                    p := hp1;
    +                  end;
    +
                     Exit;
                   end;
               end;
    +
     {$ifndef i8086}
             if CPUX86_HAS_CMOV in cpu_capabilities[current_settings.cputype] then
               begin
    @@ -2720,33 +2753,80 @@
                    end;
                  if assigned(hp1) then
                    begin
    -                  if FindLabel(tasmlabel(taicpu(p).oper[0]^.ref^.symbol),hp1) then
    +
    +                  if FindLabel(tasmlabel(symbol),hp1) then
                         begin
                           if (l<=4) and (l>0) then
                             begin
                               condition:=inverse_cond(taicpu(p).condition);
    -                          hp2:=p;
                               GetNextInstruction(p,hp1);
    -                          p:=hp1;
                               repeat
    +                            if not Assigned(hp1) then
    +                              InternalError(2018062900);
    +
                                 taicpu(hp1).opcode:=A_CMOVcc;
                                 taicpu(hp1).condition:=condition;
    +                            UpdateUsedRegs(hp1);
                                 GetNextInstruction(hp1,hp1);
    -                          until not(assigned(hp1)) or
    -                            not(CanBeCMOV(hp1));
    -                          { wait with removing else GetNextInstruction could
    -                            ignore the label if it was the only usage in the
    -                            jump moved away }
    -                          tasmlabel(taicpu(hp2).oper[0]^.ref^.symbol).decrefs;
    +                          until not(CanBeCMOV(hp1));
    +
    +                          { Don't decrement the reference count on the label yet, otherwise
    +                            GetNextInstruction might skip over the label if it drops to
    +                            zero. }
    +                          GetNextInstruction(hp1,hp2);
    +
                               { if the label refs. reach zero, remove any alignment before the label }
    -                          if (hp1.typ=ait_align) and (tasmlabel(taicpu(hp2).oper[0]^.ref^.symbol).getrefs=0) then
    +                          if (hp1.typ = ait_align) and (hp2.typ = ait_label) then
                                 begin
    -                              asml.Remove(hp1);
    -                              hp1.Free;
    +                              { Ref = 1 means it will drop to zero }
    +                              if (tasmlabel(symbol).getrefs=1) then
    +                                begin
    +                                  asml.Remove(hp1);
    +                                  hp1.Free;
    +                                end;
    +                            end
    +                          else
    +                            hp2 := hp1;
    +                                                        
    +                          if not Assigned(hp2) then
    +                            InternalError(2018062910);
    +                            
    +                          if (hp2.typ <> ait_label) then
    +                            begin
    +                              { There's something other than CMOVs here.  Move the original jump
    +                                to right before this point, then break out.
    +
    +                                Originally this was part of the above internal error, but it got
    +                                triggered on the bootstrapping process sometimes. Investigate. [Kit] }
    +                              asml.remove(p);
    +                              asml.insertbefore(p, hp2);
    +                              DebugMsg('Jcc/CMOVcc drop-out', p);
    +                              UpdateUsedRegs(p);
    +                              Result := True;
    +                              Exit;
                                 end;
    -                          asml.remove(hp2);
    -                          hp2.free;
    -                          result:=true;
    +
    +                          { Now we can safely decrement the reference count }
    +                          tasmlabel(symbol).decrefs;
    +                            
    +                          { Remove the original jump }
    +                          asml.Remove(p);
    +                          p.Free;
    +
    +                          GetNextInstruction(hp2, p); { Instruction after the label }
    +
    +                          { Remove the label if this is its final reference }
    +                          if (tasmlabel(symbol).getrefs=0) then
    +                            begin
    +                              asml.remove(hp2);
    +                              hp2.free;
    +                            end;
    +
    +                          if Assigned(p) then
    +                            begin
    +                              UpdateUsedRegs(p);
    +                              result:=true;
    +                            end;
                               exit;
                             end;
                         end
    @@ -2762,8 +2842,9 @@
                            }
                           { hp2 points to jmp yyy }
                           hp2:=hp1;
    -                      { skip hp1 to xxx }
    +                      { skip hp1 to xxx (or an align right before it) }
                           GetNextInstruction(hp1, hp1);
    +
                           if assigned(hp2) and
                             assigned(hp1) and
                             (l<=3) and
    @@ -2772,12 +2853,17 @@
                             (taicpu(hp2).condition=C_None) and
                             { real label and jump, no further references to the
                               label are allowed }
    -                        (tasmlabel(taicpu(p).oper[0]^.ref^.symbol).getrefs=1) and
    -                        FindLabel(tasmlabel(taicpu(p).oper[0]^.ref^.symbol),hp1) then
    +                        (tasmlabel(symbol).getrefs=1) and
    +                        FindLabel(tasmlabel(symbol),hp1) then
                              begin
                                l:=0;
                                { skip hp1 to <several moves 2> }
    -                           GetNextInstruction(hp1, hp1);
    +                           if (hp1.typ = ait_align) then
    +                             GetNextInstruction(hp1, hp1);
    +
    +                           GetNextInstruction(hp1, hpmov2);
    +
    +                           hp1 := hpmov2;
                                while assigned(hp1) and
                                  CanBeCMOV(hp1) do
                                  begin
    @@ -2784,47 +2870,93 @@
                                    inc(l);
                                    GetNextInstruction(hp1, hp1);
                                  end;
    -                           { hp1 points to yyy: }
    +                           { hp1 points to yyy (or an align right before it) }
    +                           hp3 := hp1;
                                if assigned(hp1) and
                                  FindLabel(tasmlabel(taicpu(hp2).oper[0]^.ref^.symbol),hp1) then
                                  begin
                                     condition:=inverse_cond(taicpu(p).condition);
                                     GetNextInstruction(p,hp1);
    -                                hp3:=p;
    -                                p:=hp1;
                                     repeat
                                       taicpu(hp1).opcode:=A_CMOVcc;
                                       taicpu(hp1).condition:=condition;
    +                                  UpdateUsedRegs(hp1);
                                       GetNextInstruction(hp1,hp1);
                                     until not(assigned(hp1)) or
                                       not(CanBeCMOV(hp1));
    -                                { hp2 is still at jmp yyy }
    -                                GetNextInstruction(hp2,hp1);
    -                                { hp2 is now at xxx: }
    +
                                     condition:=inverse_cond(condition);
    -                                GetNextInstruction(hp1,hp1);
    +                                hp1 := hpmov2;
                                     { hp1 is now at <several movs 2> }
    -                                repeat
    -                                  taicpu(hp1).opcode:=A_CMOVcc;
    -                                  taicpu(hp1).condition:=condition;
    -                                  GetNextInstruction(hp1,hp1);
    -                                until not(assigned(hp1)) or
    -                                  not(CanBeCMOV(hp1));
    -                                {
    -                                asml.remove(hp1.next)
    -                                hp1.next.free;
    +                                while Assigned(hp1) and CanBeCMOV(hp1) do
    +                                  begin
    +                                    taicpu(hp1).opcode:=A_CMOVcc;
    +                                    taicpu(hp1).condition:=condition;
    +                                    UpdateUsedRegs(hp1);
    +                                    GetNextInstruction(hp1,hp1);
    +                                  end;
    +
    +                                hp1 := p;
    +
    +                                { Get first instruction after label }
    +                                GetNextInstruction(hp3, p);
    +
    +                                if assigned(p) and (hp3.typ = ait_align) then
    +                                  GetNextInstruction(p, p);
    +
    +                                { Don't dereference yet, as doing so will cause
    +                                  GetNextInstruction to skip the label and
    +                                  optional align marker. [Kit] }
    +                                GetNextInstruction(hp2, hp4);
    +
    +                                { remove jCC }
                                     asml.remove(hp1);
                                     hp1.free;
    -                                }
    -                                { remove jCC }
    -                                tasmlabel(taicpu(hp3).oper[0]^.ref^.symbol).decrefs;
    -                                asml.remove(hp3);
    -                                hp3.free;
    +
    +                                { Remove label xxx (it will have a ref of zero due to the initial check }
    +                                if (hp4.typ = ait_align) then
    +                                  begin
    +                                    { Account for alignment as well }
    +                                    GetNextInstruction(hp4, hp1);
    +                                    asml.remove(hp1);
    +                                    hp1.free;
    +                                  end;
    +
    +                                asml.remove(hp4);
    +                                hp4.free;
    +
    +                                { Now we can safely decrement it }
    +                                tasmlabel(symbol).decrefs;
    +
                                     { remove jmp }
    -                                tasmlabel(taicpu(hp2).oper[0]^.ref^.symbol).decrefs;
    +                                symbol := taicpu(hp2).oper[0]^.ref^.symbol;
    +
                                     asml.remove(hp2);
                                     hp2.free;
    -                                result:=true;
    +
    +                                { Remove label yyy (and the optional alignment) if its reference will fall to zero }
    +                                if tasmlabel(symbol).getrefs = 1 then
    +                                  begin
    +                                    if (hp3.typ = ait_align) then
    +                                      begin
    +                                        { Account for alignment as well }
    +                                        GetNextInstruction(hp3, hp1);
    +                                        asml.remove(hp1);
    +                                        hp1.free;
    +                                      end;
    +
    +                                    asml.remove(hp3);
    +                                    hp3.free;
    +
    +                                    { As before, now we can safely decrement it }
    +                                    tasmlabel(symbol).decrefs;
    +                                  end;
    +
    +                                if Assigned(p) then
    +                                  begin
    +                                    UpdateUsedRegs(p);
    +                                    result:=true;
    +                                  end;
                                     exit;
                                  end;
                              end;
    
    i33909-recreate.patch (16,726 bytes)

Relationships

related to 0033926 closedJ. Gareth Moreton Possible internal corruption with i386-win32 -> x86_64-win64 cross compiler 
related to 0036271 resolvedFlorian [Patch] Jump optimisations in code generator 

Activities

J. Gareth Moreton

2018-06-26 06:42

developer   ~0109058

Last edited: 2018-06-26 06:51

View 3 revisions

Oh crumbs. Hmmm, okay, I'll take a look. Is it Lazarus you're cross-compiling? What's your exact operation (source platform, how you're building it etc.)?

Do-wan Kim

2018-06-26 07:21

reporter  

fix_39307_aoptx86.pas.patch (742 bytes)
Index: compiler/x86/aoptx86.pas
===================================================================
--- compiler/x86/aoptx86.pas	(revision 39307)
+++ compiler/x86/aoptx86.pas	(working copy)
@@ -2284,7 +2284,7 @@
 
             taicpu(hp2).SetCondition(SetC);
 
-            if not RegUsedAfterInstruction(taicpu(p).oper[0]^.reg, hp2, TmpUsedRegs) then
+            (* if not RegUsedAfterInstruction(taicpu(p).oper[0]^.reg, hp2, TmpUsedRegs) then
               begin
                 asml.Remove(p);
                 UpdateUsedRegs(next);
@@ -2291,7 +2291,7 @@
                 p.Free;
                 Result := True;
                 p := hp2;
-              end;
+              end; *)
 
             ReleaseUsedRegs(TmpUsedRegs);
 

Do-wan Kim

2018-06-26 07:31

reporter   ~0109060

Ignore attched file. It don't work.

I compiled fpc-lazarus by fpclazup. Base compiler is i386 and cross compile to x86-64.

J. Gareth Moreton

2018-06-26 09:06

developer   ~0109063

Last edited: 2018-06-26 09:30

View 4 revisions

I might be a beginner at this, but so far I've been unable to reproduce the problem. I've tried building 64-bit Lazarus through 32-bit Lazarus 1.8.4 with the relevant targets (and I checked it was using ppcrossx64.exe), and tried compiling the culprit file, ttdebug.pas, through the cross compiler while said compiler was itself open in the debugger, with nothing untoward. I'm possibly missing something, but can you lay out, step-by-step, what you did to generate the cross compiler and how you went about compiling fpc-lazarus (and which version) in order to produce the error?

Given your patch doesn't fix the problem, I wonder if the problem does not lie with that particular optimisation. Nevertheless, I updated my local repository to the SVN head prior to looking at this issue - does the crash still occur for you?

Do-wan Kim

2018-06-26 16:31

reporter  

fix-corss64error-aoptx86.pas.patch (926 bytes)
Index: compiler/x86/aoptx86.pas
===================================================================
--- compiler/x86/aoptx86.pas	(revision 39311)
+++ compiler/x86/aoptx86.pas	(working copy)
@@ -2686,7 +2686,7 @@
                   end;
               end;
 
-            if (hp1.typ=ait_label) and (TAsmLabel(taicpu(p).oper[0]^.ref^.symbol) = tai_label(hp1).labsym) then
+            (* if (hp1.typ=ait_label) and (TAsmLabel(taicpu(p).oper[0]^.ref^.symbol) = tai_label(hp1).labsym) then
               begin
                 { If Jcc is immediately followed by the label that it's supposed to jump to, remove it }
                 UpdateUsedRegs(tai(p.next));
@@ -2698,7 +2698,7 @@
                 Result := True;
                 p := hp1;
                 Exit;
-              end;
+              end; *)
           end;
 {$ifndef i8086}
         if CPUX86_HAS_CMOV in cpu_capabilities[current_settings.cputype] then

Do-wan Kim

2018-06-26 16:37

reporter   ~0109068

Last edited: 2018-06-27 02:00

View 3 revisions

I found problem is 'Removed conditional jump whose destination was immediately after it' optimization. I tested twice cross compiling with disabled it.

I have using ini file for fpclazup.
-------------------------------------------------------
[normal]
fpcurl=https://svn.freepascal.org/svn/fpc/trunk
lazurl=https://svn.freepascal.org/svn/lazarus/trunk
fpcopt="-dTEST_WIN32_SEH"
lazopt="-ddisableUTF8RTL"
fpcuplinkname=fpcuptrunk
lazlinkname=lazarus_fpcup
keeplocalchanges=true
getfullrepo=true
-------------------------------------------------------

(edit)
I'm also using cross compile batch for lazarus. Sometimes fpclazup(20170616) don't success with lazarus cross compiling(fpclazup problem but ok).


cd lazarus
C:\development\lazarus\lazbuild.exe --quiet --pcp=C:\development\config_lazarus --cpu=x86_64 --os=win64 packager\registration\fcl.lpk components\lazutils\lazutils.lpk lcl\interfaces\lcl.lpk components\synedit\synedit.lpk components\lazcontrols\lazcontrols.lpk components\ideintf\ideintf.lpk
cd ..

(edit)
My lazarus compile option.
-g -gl -O2 -Xs -CX -XX -ddisableUTF8RTL

J. Gareth Moreton

2018-06-27 03:52

developer   ~0109078

Last edited: 2018-06-27 03:52

View 2 revisions

This could be interesting to get to the bottom of. Removing the jump when you have "Jcc @@lbl; @@lbl:" should not logically change the program's flow. I'll see what I can find.

J. Gareth Moreton

2018-06-27 14:22

developer   ~0109084

Reproduced the crash, and also managed to further isolate it by running the following: \pp\bin\i386-win32\ppcrossx64.exe components\lazutils\ttdebug.pas -g -gl -O2 -Xs -CX -XX -ddisableUTF8RTL

Note: DWARF debug information cannot be used with smart linking on this target, switching to static linking
Free Pascal Compiler version 3.1.1 [2018/06/26] for x86_64
Copyright (c) 1993-2018 by Florian Klaempfl and others
Target OS: Win64 for x64
Compiling components\lazutils\ttdebug.pas
ttdebug.pas(653,8) Error: Compilation raised exception internally
Fatal: Compilation aborted
An unhandled exception occurred at $00006E67:
EAccessViolation: Access violation
  $00006E67
  $00441B2E
  $004421DF
  $005757CE
  $00577E7E
  $00577823
  $00435412
  $00414794

That means I can now run the compiler in the debugger to better diagnose the fault.

J. Gareth Moreton

2018-06-29 07:19

developer   ~0109123

Last edited: 2018-06-29 07:20

View 2 revisions

Okay, there's something strange going on. I disabled my changes but the above crash still occurs, and according to the assembly for ttdebug, there actually aren't any Jcc or SETcc optimisations occurring (in one situation where it compiles in the debugger, and analysing the code for any potential optimisations).

J. Gareth Moreton

2018-06-29 15:42

developer   ~0109128

I believe I have fixed the bug. It was actually with the CMOV optimisations, not my Jcc addition. Find attached i33909.patch - hopefully it works.

I've also added in some extra internal errors to help catch the crashes.

Do-wan Kim

2018-06-30 02:00

reporter   ~0109133

i33909.patch works good. No more cross compiling problem.

J. Gareth Moreton

2018-06-30 07:15

developer   ~0109136

Excellent - will assign to Florian to evaluate and merge with the head.

Florian

2018-06-30 09:16

administrator   ~0109138

It is not resolved yet, right? So I marked it as new again.

Was the patch generated against a clean working copy? I cannot apply it?

J. Gareth Moreton

2018-06-30 10:31

developer   ~0109139

It should have been. Let me have another look.

J. Gareth Moreton

2018-06-30 10:31

developer  

i33909-recreate.patch (16,726 bytes)
Index: compiler/aoptobj.pas
===================================================================
--- compiler/aoptobj.pas	(revision 39346)
+++ compiler/aoptobj.pas	(working copy)
@@ -1671,7 +1671,6 @@
         ClearUsedRegs;
         while (p <> BlockEnd) Do
           begin
-            UpdateUsedRegs(tai(p.next));
             if PeepHoleOptPass2Cpu(p) then
               continue;
             if assigned(p) then
Index: compiler/ogbase.pas
===================================================================
--- compiler/ogbase.pas	(revision 39346)
+++ compiler/ogbase.pas	(working copy)
@@ -1351,6 +1351,9 @@
       begin
         if assigned(asmsym) then
           begin
+            if asmsym.typ = AT_NONE then
+              InternalError(2018062800);
+
             if not assigned(asmsym.cachedObjSymbol) then
               begin
                 result:=symboldefine(asmsym.name,asmsym.bind,asmsym.typ);
Index: compiler/x86/aoptx86.pas
===================================================================
--- compiler/x86/aoptx86.pas	(revision 39346)
+++ compiler/x86/aoptx86.pas	(working copy)
@@ -2240,6 +2240,7 @@
         hp1,hp2,next: tai; SetC, JumpC: TAsmCond;
       begin
         Result:=false;
+
         if MatchOpType(taicpu(p),top_reg) and
           GetNextInstruction(p, hp1) and
           MatchInstruction(hp1, A_TEST, [S_B]) and
@@ -2622,26 +2623,29 @@
 
     function TX86AsmOptimizer.OptPass2Jcc(var p : tai) : boolean;
       var
-        hp1,hp2,hp3: tai;
+        hp1,hp2,hp3,hp4,hpmov2: tai;
         carryadd_opcode : TAsmOp;
         l : Longint;
         condition : TAsmCond;
+        symbol: TAsmSymbol;
       begin
         result:=false;
         if GetNextInstruction(p,hp1) then
           begin
+            symbol := TAsmLabel(taicpu(p).oper[0]^.ref^.symbol);
+
             if (hp1.typ=ait_instruction) and
                GetNextInstruction(hp1,hp2) and (hp2.typ=ait_label) and
-               (Tasmlabel(Taicpu(p).oper[0]^.ref^.symbol)=Tai_label(hp2).labsym) then
+               (Tasmlabel(symbol) = Tai_label(hp2).labsym) then
                  { jb @@1                            cmc
                    inc/dec operand           -->     adc/sbb operand,0
-         	@@1:
+                   @@1:
 
-         	... and ...
+                   ... and ...
 
                    jnb @@1
                    inc/dec operand           -->     adc/sbb operand,0
-         	@@1: }
+                   @@1: }
               begin
                 carryadd_opcode:=A_NONE;
                 if Taicpu(p).condition in [C_NAE,C_B] then
@@ -2686,20 +2690,49 @@
                   end;
               end;
 
-            if (hp1.typ=ait_label) and (TAsmLabel(taicpu(p).oper[0]^.ref^.symbol) = tai_label(hp1).labsym) then
+            if ((hp1.typ = ait_label) and (symbol = tai_label(hp1).labsym))
+                or ((hp1.typ = ait_align) and GetNextInstruction(hp1, hp2) and (hp2.typ = ait_label) and (symbol = tai_label(hp2).labsym)) then
               begin
                 { If Jcc is immediately followed by the label that it's supposed to jump to, remove it }
-                UpdateUsedRegs(tai(p.next));
-
                 DebugMsg(SPeepholeOptimization + 'Removed conditional jump whose destination was immediately after it', p);
+                UpdateUsedRegs(hp1);
 
+                TAsmLabel(symbol).decrefs;
+                { if the label refs. reach zero, remove any alignment before the label }
+                if (hp1.typ = ait_align) then
+                  begin
+                    UpdateUsedRegs(hp2);
+                    if (TAsmLabel(symbol).getrefs = 0) then
+                    begin
+                      asml.Remove(hp1);
+                      hp1.Free;
+                    end;
+                    hp1 := hp2; { Set hp1 to the label }
+                  end;
+
                 asml.remove(p);
                 p.free;
-                Result := True;
-                p := hp1;
+
+                if (TAsmLabel(symbol).getrefs = 0) then
+                  begin
+                    GetNextInstruction(hp1, p); { Instruction following the label }
+                    asml.remove(hp1);
+                    hp1.free;
+
+                    UpdateUsedRegs(p);
+                    Result := True;
+                  end
+                else
+                  begin
+                    { We don't need to set the result to True because we know hp1
+                      is a label and won't trigger any optimisation routines. [Kit] }
+                    p := hp1;
+                  end;
+
                 Exit;
               end;
           end;
+
 {$ifndef i8086}
         if CPUX86_HAS_CMOV in cpu_capabilities[current_settings.cputype] then
           begin
@@ -2720,33 +2753,80 @@
                end;
              if assigned(hp1) then
                begin
-                  if FindLabel(tasmlabel(taicpu(p).oper[0]^.ref^.symbol),hp1) then
+
+                  if FindLabel(tasmlabel(symbol),hp1) then
                     begin
                       if (l<=4) and (l>0) then
                         begin
                           condition:=inverse_cond(taicpu(p).condition);
-                          hp2:=p;
                           GetNextInstruction(p,hp1);
-                          p:=hp1;
                           repeat
+                            if not Assigned(hp1) then
+                              InternalError(2018062900);
+
                             taicpu(hp1).opcode:=A_CMOVcc;
                             taicpu(hp1).condition:=condition;
+                            UpdateUsedRegs(hp1);
                             GetNextInstruction(hp1,hp1);
-                          until not(assigned(hp1)) or
-                            not(CanBeCMOV(hp1));
-                          { wait with removing else GetNextInstruction could
-                            ignore the label if it was the only usage in the
-                            jump moved away }
-                          tasmlabel(taicpu(hp2).oper[0]^.ref^.symbol).decrefs;
+                          until not(CanBeCMOV(hp1));
+
+                          { Don't decrement the reference count on the label yet, otherwise
+                            GetNextInstruction might skip over the label if it drops to
+                            zero. }
+                          GetNextInstruction(hp1,hp2);
+
                           { if the label refs. reach zero, remove any alignment before the label }
-                          if (hp1.typ=ait_align) and (tasmlabel(taicpu(hp2).oper[0]^.ref^.symbol).getrefs=0) then
+                          if (hp1.typ = ait_align) and (hp2.typ = ait_label) then
                             begin
-                              asml.Remove(hp1);
-                              hp1.Free;
+                              { Ref = 1 means it will drop to zero }
+                              if (tasmlabel(symbol).getrefs=1) then
+                                begin
+                                  asml.Remove(hp1);
+                                  hp1.Free;
+                                end;
+                            end
+                          else
+                            hp2 := hp1;
+                                                        
+                          if not Assigned(hp2) then
+                            InternalError(2018062910);
+                            
+                          if (hp2.typ <> ait_label) then
+                            begin
+                              { There's something other than CMOVs here.  Move the original jump
+                                to right before this point, then break out.
+
+                                Originally this was part of the above internal error, but it got
+                                triggered on the bootstrapping process sometimes. Investigate. [Kit] }
+                              asml.remove(p);
+                              asml.insertbefore(p, hp2);
+                              DebugMsg('Jcc/CMOVcc drop-out', p);
+                              UpdateUsedRegs(p);
+                              Result := True;
+                              Exit;
                             end;
-                          asml.remove(hp2);
-                          hp2.free;
-                          result:=true;
+
+                          { Now we can safely decrement the reference count }
+                          tasmlabel(symbol).decrefs;
+                            
+                          { Remove the original jump }
+                          asml.Remove(p);
+                          p.Free;
+
+                          GetNextInstruction(hp2, p); { Instruction after the label }
+
+                          { Remove the label if this is its final reference }
+                          if (tasmlabel(symbol).getrefs=0) then
+                            begin
+                              asml.remove(hp2);
+                              hp2.free;
+                            end;
+
+                          if Assigned(p) then
+                            begin
+                              UpdateUsedRegs(p);
+                              result:=true;
+                            end;
                           exit;
                         end;
                     end
@@ -2762,8 +2842,9 @@
                        }
                       { hp2 points to jmp yyy }
                       hp2:=hp1;
-                      { skip hp1 to xxx }
+                      { skip hp1 to xxx (or an align right before it) }
                       GetNextInstruction(hp1, hp1);
+
                       if assigned(hp2) and
                         assigned(hp1) and
                         (l<=3) and
@@ -2772,12 +2853,17 @@
                         (taicpu(hp2).condition=C_None) and
                         { real label and jump, no further references to the
                           label are allowed }
-                        (tasmlabel(taicpu(p).oper[0]^.ref^.symbol).getrefs=1) and
-                        FindLabel(tasmlabel(taicpu(p).oper[0]^.ref^.symbol),hp1) then
+                        (tasmlabel(symbol).getrefs=1) and
+                        FindLabel(tasmlabel(symbol),hp1) then
                          begin
                            l:=0;
                            { skip hp1 to <several moves 2> }
-                           GetNextInstruction(hp1, hp1);
+                           if (hp1.typ = ait_align) then
+                             GetNextInstruction(hp1, hp1);
+
+                           GetNextInstruction(hp1, hpmov2);
+
+                           hp1 := hpmov2;
                            while assigned(hp1) and
                              CanBeCMOV(hp1) do
                              begin
@@ -2784,47 +2870,93 @@
                                inc(l);
                                GetNextInstruction(hp1, hp1);
                              end;
-                           { hp1 points to yyy: }
+                           { hp1 points to yyy (or an align right before it) }
+                           hp3 := hp1;
                            if assigned(hp1) and
                              FindLabel(tasmlabel(taicpu(hp2).oper[0]^.ref^.symbol),hp1) then
                              begin
                                 condition:=inverse_cond(taicpu(p).condition);
                                 GetNextInstruction(p,hp1);
-                                hp3:=p;
-                                p:=hp1;
                                 repeat
                                   taicpu(hp1).opcode:=A_CMOVcc;
                                   taicpu(hp1).condition:=condition;
+                                  UpdateUsedRegs(hp1);
                                   GetNextInstruction(hp1,hp1);
                                 until not(assigned(hp1)) or
                                   not(CanBeCMOV(hp1));
-                                { hp2 is still at jmp yyy }
-                                GetNextInstruction(hp2,hp1);
-                                { hp2 is now at xxx: }
+
                                 condition:=inverse_cond(condition);
-                                GetNextInstruction(hp1,hp1);
+                                hp1 := hpmov2;
                                 { hp1 is now at <several movs 2> }
-                                repeat
-                                  taicpu(hp1).opcode:=A_CMOVcc;
-                                  taicpu(hp1).condition:=condition;
-                                  GetNextInstruction(hp1,hp1);
-                                until not(assigned(hp1)) or
-                                  not(CanBeCMOV(hp1));
-                                {
-                                asml.remove(hp1.next)
-                                hp1.next.free;
+                                while Assigned(hp1) and CanBeCMOV(hp1) do
+                                  begin
+                                    taicpu(hp1).opcode:=A_CMOVcc;
+                                    taicpu(hp1).condition:=condition;
+                                    UpdateUsedRegs(hp1);
+                                    GetNextInstruction(hp1,hp1);
+                                  end;
+
+                                hp1 := p;
+
+                                { Get first instruction after label }
+                                GetNextInstruction(hp3, p);
+
+                                if assigned(p) and (hp3.typ = ait_align) then
+                                  GetNextInstruction(p, p);
+
+                                { Don't dereference yet, as doing so will cause
+                                  GetNextInstruction to skip the label and
+                                  optional align marker. [Kit] }
+                                GetNextInstruction(hp2, hp4);
+
+                                { remove jCC }
                                 asml.remove(hp1);
                                 hp1.free;
-                                }
-                                { remove jCC }
-                                tasmlabel(taicpu(hp3).oper[0]^.ref^.symbol).decrefs;
-                                asml.remove(hp3);
-                                hp3.free;
+
+                                { Remove label xxx (it will have a ref of zero due to the initial check }
+                                if (hp4.typ = ait_align) then
+                                  begin
+                                    { Account for alignment as well }
+                                    GetNextInstruction(hp4, hp1);
+                                    asml.remove(hp1);
+                                    hp1.free;
+                                  end;
+
+                                asml.remove(hp4);
+                                hp4.free;
+
+                                { Now we can safely decrement it }
+                                tasmlabel(symbol).decrefs;
+
                                 { remove jmp }
-                                tasmlabel(taicpu(hp2).oper[0]^.ref^.symbol).decrefs;
+                                symbol := taicpu(hp2).oper[0]^.ref^.symbol;
+
                                 asml.remove(hp2);
                                 hp2.free;
-                                result:=true;
+
+                                { Remove label yyy (and the optional alignment) if its reference will fall to zero }
+                                if tasmlabel(symbol).getrefs = 1 then
+                                  begin
+                                    if (hp3.typ = ait_align) then
+                                      begin
+                                        { Account for alignment as well }
+                                        GetNextInstruction(hp3, hp1);
+                                        asml.remove(hp1);
+                                        hp1.free;
+                                      end;
+
+                                    asml.remove(hp3);
+                                    hp3.free;
+
+                                    { As before, now we can safely decrement it }
+                                    tasmlabel(symbol).decrefs;
+                                  end;
+
+                                if Assigned(p) then
+                                  begin
+                                    UpdateUsedRegs(p);
+                                    result:=true;
+                                  end;
                                 exit;
                              end;
                          end;
i33909-recreate.patch (16,726 bytes)

J. Gareth Moreton

2018-06-30 10:32

developer   ~0109140

Seems that one of the removals got missed out of the patch - try this one.

There are some minor changes in tabbing in places because I used Notepad++ to change all tabs into spaces (since I was using it as an editor at one point).

Do-wan Kim

2018-06-30 13:29

reporter   ~0109143

i33909-recreate.patch works good.

Florian

2018-07-01 14:57

administrator   ~0109154

Thanks for figuring this out, applied.

Do-wan Kim

2018-07-02 00:57

reporter   ~0109174

Thank you!

Issue History

Date Modified Username Field Change
2018-06-26 02:41 Do-wan Kim New Issue
2018-06-26 06:42 J. Gareth Moreton Note Added: 0109058
2018-06-26 06:42 J. Gareth Moreton Note Edited: 0109058 View Revisions
2018-06-26 06:50 J. Gareth Moreton Assigned To => J. Gareth Moreton
2018-06-26 06:50 J. Gareth Moreton Status new => assigned
2018-06-26 06:51 J. Gareth Moreton Priority normal => urgent
2018-06-26 06:51 J. Gareth Moreton Severity minor => crash
2018-06-26 06:51 J. Gareth Moreton Note Edited: 0109058 View Revisions
2018-06-26 07:21 Do-wan Kim File Added: fix_39307_aoptx86.pas.patch
2018-06-26 07:31 Do-wan Kim Note Added: 0109060
2018-06-26 09:06 J. Gareth Moreton Note Added: 0109063
2018-06-26 09:07 J. Gareth Moreton Status assigned => feedback
2018-06-26 09:07 J. Gareth Moreton Resolution open => unable to reproduce
2018-06-26 09:08 J. Gareth Moreton Note Edited: 0109063 View Revisions
2018-06-26 09:08 J. Gareth Moreton Note Edited: 0109063 View Revisions
2018-06-26 09:30 J. Gareth Moreton Note Edited: 0109063 View Revisions
2018-06-26 16:31 Do-wan Kim File Added: fix-corss64error-aoptx86.pas.patch
2018-06-26 16:37 Do-wan Kim Note Added: 0109068
2018-06-26 16:37 Do-wan Kim Status feedback => assigned
2018-06-27 01:39 Do-wan Kim Note Edited: 0109068 View Revisions
2018-06-27 02:00 Do-wan Kim Note Edited: 0109068 View Revisions
2018-06-27 03:52 J. Gareth Moreton Note Added: 0109078
2018-06-27 03:52 J. Gareth Moreton Summary New SETcc optimization makes error => New Jcc optimization makes error
2018-06-27 03:52 J. Gareth Moreton Description Updated View Revisions
2018-06-27 03:52 J. Gareth Moreton Note Edited: 0109078 View Revisions
2018-06-27 14:22 J. Gareth Moreton Note Added: 0109084
2018-06-27 14:24 J. Gareth Moreton Resolution unable to reproduce => open
2018-06-29 07:19 J. Gareth Moreton Note Added: 0109123
2018-06-29 07:20 J. Gareth Moreton Note Edited: 0109123 View Revisions
2018-06-29 10:24 J. Gareth Moreton Relationship added related to 0033926
2018-06-29 15:40 J. Gareth Moreton File Added: i33909.patch
2018-06-29 15:42 J. Gareth Moreton Note Added: 0109128
2018-06-29 15:42 J. Gareth Moreton Assigned To J. Gareth Moreton =>
2018-06-29 15:42 J. Gareth Moreton Status assigned => feedback
2018-06-29 15:57 J. Gareth Moreton File Deleted: i33909.patch
2018-06-29 15:58 J. Gareth Moreton File Added: i33909.patch
2018-06-29 16:04 J. Gareth Moreton Assigned To => J. Gareth Moreton
2018-06-30 02:00 Do-wan Kim Note Added: 0109133
2018-06-30 02:00 Do-wan Kim Status feedback => assigned
2018-06-30 07:12 J. Gareth Moreton File Deleted: i33909.patch
2018-06-30 07:12 J. Gareth Moreton File Added: i33909.patch
2018-06-30 07:13 J. Gareth Moreton Assigned To J. Gareth Moreton => Florian
2018-06-30 07:15 J. Gareth Moreton Note Added: 0109136
2018-06-30 07:42 J. Gareth Moreton Status assigned => resolved
2018-06-30 07:42 J. Gareth Moreton Resolution open => fixed
2018-06-30 07:42 J. Gareth Moreton Target Version => 3.1.1
2018-06-30 09:12 Florian Assigned To Florian =>
2018-06-30 09:12 Florian Status resolved => new
2018-06-30 09:16 Florian Note Added: 0109138
2018-06-30 10:31 J. Gareth Moreton Note Added: 0109139
2018-06-30 10:31 J. Gareth Moreton File Deleted: i33909.patch
2018-06-30 10:31 J. Gareth Moreton File Added: i33909-recreate.patch
2018-06-30 10:32 J. Gareth Moreton Note Added: 0109140
2018-06-30 10:32 J. Gareth Moreton Status new => resolved
2018-06-30 10:32 J. Gareth Moreton Assigned To => Florian
2018-06-30 11:34 J. Gareth Moreton Status resolved => feedback
2018-06-30 11:34 J. Gareth Moreton Resolution fixed => reopened
2018-06-30 11:34 J. Gareth Moreton Resolution reopened => fixed
2018-06-30 13:29 Do-wan Kim Note Added: 0109143
2018-06-30 13:29 Do-wan Kim Status feedback => assigned
2018-07-01 14:57 Florian Fixed in Revision => 39353
2018-07-01 14:57 Florian Note Added: 0109154
2018-07-01 14:57 Florian Status assigned => resolved
2018-07-01 14:57 Florian Fixed in Version => 3.1.1
2018-07-02 00:57 Do-wan Kim Note Added: 0109174
2018-07-02 00:57 Do-wan Kim Status resolved => closed
2019-11-09 21:26 J. Gareth Moreton Relationship added related to 0036271