View Issue Details
|ID||Project||Category||View Status||Date Submitted||Last Update|
|0011327||FPC||Compiler||public||2008-05-20 17:38||2010-12-02 10:28|
|Reporter||Sergei Gorelkin||Assigned To||Florian|
|Fixed in Version||2.4.0|
|Summary||0011327: Inefficient code generated for Pos('ascii_literal', WideStringVariable)|
|Description||When 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.
|Fixed in Revision||11636|
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.
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.
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.
||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)|
t.pas (1,311 bytes)
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.
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.
> 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.
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.
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)
||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.|
|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|