View Issue Details

IDProjectCategoryView StatusLast Update
0011327FPCCompilerpublic2010-12-02 10:28
ReporterSergei Gorelkin Assigned ToFlorian  
PrioritynormalSeverityfeatureReproducibilityalways
Status feedbackResolutionreopened 
Product Version2.3.1 
Fixed in Version2.4.0 
Summary0011327: Inefficient code generated for Pos('ascii_literal', WideStringVariable)
DescriptionWhen compiling calls like Pos('abc', WideStringVar), the compiler generates an AnsiString constant and code which converts it into WideString temporary variable before call to Pos(), instead of using a WideString constant directly (as Delphi does in this case). The code remains correct, but its performance drops significantly.

Unfortunately, I don't know how to write a complete test case for this.
Tagsunicode
Fixed in Revision11636
FPCOldBugId0
FPCTarget-
Attached Files

Activities

Sergei Gorelkin

2008-10-15 21:58

developer   ~0022755

Looks like it got broken again since r11636. Testing yesterday's r11901 shows that the old inefficient code is generated.

However, I finally realized that it is possible to test this case in automated way by hooking into WideStringManager. The test program is attached.

Sergei Gorelkin

2008-11-02 20:18

developer   ~0023097

I found out that the exact reason is the change of tstringconstnode.expectloc from LOC_CREFERENCE to LOC_REGISTER which happened in r11666 and then merged into trunk in r11739. (in function tstringconstnode.pass_1, file ncon.pas).

While LOC_REGISTER is of course more correct value, it seems to be considered a non-constant and therefore does not propagate during constant folding phase.

Sergei Gorelkin

2009-04-10 12:09

developer   ~0026690

Here's some more details on what's happening:

1) An incorrect overloaded function is selected. The overload selector has a bad day here, because no reliable match can be done on the first argument (the second arg actually matters). The first "conststring" arg matches AnsiString and WideString equally, and AnsiString is being chosen in order to "avoid data loss", as stated in the source comments.
2) The Pos(AnsiString, WideString) is inlined, and that's where the temp var and conversion code actually come from.
3) My own patch that removes an unneeded pointer to the string and therefore changes the location of string const node from LOC_CREFERENCE to LOC_REGISTER also plays certain role. While LOC_CREFERENCE propagates through typeconv during constant folding, LOC_REGISTER does not. Note that the patch initially contained LOC_CREGISTER, and I didn't have regressions with that. However, it was later changed to LOC_REGISTER - probably not without reason.

Also it looks for me that the compiler lacks the location type that represents an address (LOC_[C]REFERENCE represents the content of a memory cell, LOC_CONSTANT is an immediate operand, the rest are various flavours of CPU registers). Therefore, locations of the strings have to be "emulated" with regiters, by emitting code which loads the register with necessary address.
This applies not only to strings - I've seen many similar places in compiler code. Using "address location" could possibly reduce register pressure and result in more efficient code, but that's very IMHO.

Jonas Maebe

2010-11-30 13:38

manager   ~0043797

It currently works fine for me in trunk on Mac OS X (if you replace "twidestringmanager" with "tunicodestringmanager", because twidestringmanager no longer exists on unix platforms)

2010-11-30 14:18

 

t.pas (1,311 bytes)

Sergei Gorelkin

2010-11-30 14:26

developer   ~0043804

Last edited: 2010-11-30 14:31

No, it doesn't :(
The test itself was not very correct, it was missing a hook into Ansi2UnicodeMoveProc. Besides, it was halting right after checking WideString, without performing UnicodeString check. So indeed you would get a no-error result in non-Windows.
I replaced the test with updated one, which currently fails both assertions in Windows, and should fail UnicodeString assertion in non-Windows.

Jonas Maebe

2010-11-30 16:48

manager   ~0043813

One comment about point 1 in comment 26690: what's not preferred due to the danger of losing data (comment in defcmp.pas:339), is converting a widestring/unicodestring constant to an ansistring/shortstring.

Converting ansistring/shortstring constants to widestring/unicodestring is not done unless required because that would mean that if you have pos('abc',ansistringvar), the compiler would choose the "pos(unicodestring,ansistring)" overload instead of "pos(ansistring,ansistring)".

Regarding comment 3, LOC_CREGISTER ("constant register") means that a register contains a register variable and therefore cannot be modified, except when actually performing an operation on this register variable (because other expressions may still use it later and expect it to still contain its original value). A LOC_CREGISTER results therefore generally in more register moves and register pressure than a LOC_REGISTER.

What do you mean by "LOC_CREFERENCE propagates through typeconv during constant folding"? I can't find any code that cares about this regarding strings.

Also, regarding your last remark in that comment, there is LOC_(C)REFERENCE with ref.refaddr=addr_full, which means "the address of the LOC_(C)REFERENCE, not the contents of the memory it refers to". That is however only used in a few places in the compiler, and only at the code generator level for consumption by the assembler writers. There's no support for this at the node tree level (that code doesn't look at the refaddr field), so it probably can't really be used without lots of modifications everywhere.

I guess a solution could be to make converting a constant string to a wide/unicodestring more preferred than converting a non-constant wide/unicodestring to an ansistring. Changing the convert levels is always tricky though, because you can easily end up with lots of new "cannot choose which overloaded function to call" errors, or even completely wrong overload choices.

Sergei Gorelkin

2010-11-30 18:21

developer   ~0043816

> What do you mean by "LOC_CREFERENCE propagates through typeconv during constant folding"? I can't find any code that cares about this regarding strings.

Originally string literals were processed like typed constants (generating a pointer location containing a pointer to actual string data). Location of stringconstn was LOC_CREFERENCE. Florian originally fixed this issue in r11636 by improving constant folding for typeconvnode. Then I suggested a patch which removed an extra indirection and switched to using a direct pointer to string data. Location of stringconstn changed to LOC_REGISTER, this issue was not broken. However, due to insufficient knowledge of compiler internals at that point I missed setting the same expectloc in pass1, and it remained LOC_CREFERENCE. That did not seem to cause any ill-effects, except warnings when compiled with EXTDEBUG defined. Eventually (in rev. 11739) that was fixed, too - at that point constant folding got broken. I also failed to find out how exactly it happens, it's just a matter of fact that setting tstringconstnode.expectloc=LOC_CREFERENCE in pass1 works but LOC_REGISTER does not.

Sergei Gorelkin

2010-12-02 09:55

developer   ~0043876

Found the reason: expectloc is checked in tcallnode.createinlineparas, which puts parameter to temp in case it's located in register.

I tried the approach with ref.refaddr=addr_full, and it appears to work just fine on i386. Of course, other CPUs need testing. Attaching a proposed patch.

2010-12-02 09:56

 

11327.patch (2,032 bytes)   
Index: ncgcon.pas
===================================================================
--- ncgcon.pas	(revision 16486)
+++ ncgcon.pas	(working copy)
@@ -265,7 +265,6 @@
          lastlabel   : tasmlabel;
          pc       : pchar;
          l: longint;
-         href: treference;
          pooltype: TConstPoolType;
          pool: THashSet;
          entry: PHashSetItem;
@@ -360,18 +359,12 @@
                    entry^.Data:=lastlabel;
                 end;
            end;
+
+         location_reset_ref(location, LOC_CREFERENCE, def_cgsize(resultdef), const_align(sizeof(pint)));
+         location.reference.symbol:=lab_str;
          if cst_type in [cst_ansistring, cst_widestring, cst_unicodestring] then
-           begin
-             location_reset(location, LOC_REGISTER, OS_ADDR);
-             reference_reset_symbol(href, lab_str, 0, const_align(sizeof(pint)));
-             location.register:=cg.getaddressregister(current_asmdata.CurrAsmList);
-             cg.a_loadaddr_ref_reg(current_asmdata.CurrAsmList,href,location.register);
-           end
-         else
-           begin
-             location_reset_ref(location, LOC_CREFERENCE, def_cgsize(resultdef), const_align(sizeof(pint)));
-             location.reference.symbol:=lab_str;
-           end;
+           location.reference.refaddr:=addr_full;
+
       end;
 
 
Index: ncon.pas
===================================================================
--- ncon.pas	(revision 16486)
+++ ncon.pas	(working copy)
@@ -939,13 +939,8 @@
     function tstringconstnode.pass_1 : tnode;
       begin
         result:=nil;
-        if (cst_type in [cst_ansistring,cst_widestring,cst_unicodestring]) then
-          begin
-            if len=0 then
-              expectloc:=LOC_CONSTANT
-            else
-              expectloc:=LOC_REGISTER
-          end
+        if (cst_type in [cst_ansistring,cst_widestring,cst_unicodestring]) and (len=0) then
+          expectloc:=LOC_CONSTANT
         else
           expectloc:=LOC_CREFERENCE;
       end;
11327.patch (2,032 bytes)   

Jonas Maebe

2010-12-02 10:28

manager   ~0043878

It's not just other CPUs that need testing. Because the node-level code generator doesn't know about this at all, it may break at any time due to any change. I also don't think the assembler level optimizers know about it (since it's currently only used for jumps/calls, that doesn't matter). Therefore I don't think it's possible to use it like that.

Issue History

Date Modified Username Field Change
2008-05-20 17:38 Sergei Gorelkin New Issue
2008-05-20 18:03 Jonas Maebe FPCOldBugId => 0
2008-05-20 18:03 Jonas Maebe FPCTarget => -
2008-05-20 18:03 Jonas Maebe Severity minor => feature
2008-06-15 22:40 Marco van de Voort Tag Attached: unicode
2008-08-23 09:34 Florian Fixed in Revision => 11636
2008-08-23 09:34 Florian Status new => resolved
2008-08-23 09:34 Florian Fixed in Version => 2.3.1
2008-08-23 09:34 Florian Resolution open => fixed
2008-08-23 09:34 Florian Assigned To => Florian
2008-10-15 21:58 Sergei Gorelkin Status resolved => feedback
2008-10-15 21:58 Sergei Gorelkin Resolution fixed => reopened
2008-10-15 21:58 Sergei Gorelkin Note Added: 0022755
2008-10-15 21:58 Sergei Gorelkin File Added: t.pas
2008-11-02 20:18 Sergei Gorelkin Note Added: 0023097
2009-04-10 12:09 Sergei Gorelkin Note Added: 0026690
2010-11-30 13:38 Jonas Maebe Note Added: 0043797
2010-11-30 14:17 Sergei Gorelkin File Deleted: t.pas
2010-11-30 14:18 Sergei Gorelkin File Added: t.pas
2010-11-30 14:26 Sergei Gorelkin Note Added: 0043804
2010-11-30 14:31 Sergei Gorelkin Note Edited: 0043804
2010-11-30 16:48 Jonas Maebe Note Added: 0043813
2010-11-30 18:21 Sergei Gorelkin Note Added: 0043816
2010-12-02 09:55 Sergei Gorelkin Note Added: 0043876
2010-12-02 09:56 Sergei Gorelkin File Added: 11327.patch
2010-12-02 10:28 Jonas Maebe Note Added: 0043878