<HTML>
<style> BODY { font-family:Arial, Helvetica, sans-serif;font-size:12px; }</style>Well, at least we're having a good civil conversation about it! As long as we can talk and discuss things, then there's no harm, and hopefully someone like me who is over-enthusiastic at times can still be useful!<br>
<br>
<div>I suppose loc[low(loc)] isn't necessarily bad and is guaranteed to be correct - just feels a little clumsy, that's all, plus with the almost universal standard of starting arrays at 0, low(loc) seems a bit superfluous (although looks neat when paired with "High", e.g. "for x := low(loc) to high(loc)" etc.)</div><div><br>
</div><div>All I can promise is that the code I write tends to be completely original. I was exploring cpupara.pas with no knowledge of libffi, so looking for optimisations with fresh eyes. Admittedly I was looking there while researching something else and just saw something and thought "that looks a bit inefficient" and also was a little bemused at the mixed standards (i.e. loc[0] and loc[low(loc)], and also using "fillchar" and a for-loop to initialise an array of the same record type).<br>
<br>
I try to avoid hacks and produce clean code, or if I have to use something a little strange, like calling Create as a standard method rather than as a constructor (I did this in my patch for #34679), I make sure the comments explain clearly what's going on.</div><div><br>
</div><div>If I had to describe my own standard, it would be "iterative improvement". I start with something rough (although hopefully still relatively clean according to a design plan) and then refactor it over time after leaving it for a bit, or in the case of FPC, coming to the code with no previous knowledge of it and studying how it works. I also look for small performance gains that, by themselves, don't amount to much, but when several of them stack together, start to have a noticeable effect.<br>
</div><div><br>
</div><div>My biggest submission to date, but which is still pending analysis and approval by Florian and others, is the x86-64 optimiser overhaul (although it also affects i386 now as well, since separating out the 32-bit code for testing reasons just made everything incredibly untidy with $ifdef's). That one is a case of "if it ain't broke, don't fix it", although given my preliminary tests show a 15% speed increase, it's a little bit deeper than just fixing it! The main objective with the overhaul was to reduce the number of passes over individual assembler blocks - currently it's a minimum of 5 (pre-peephole, pass 1, pass 1 again, pass 2 and post-peephole), while the overhaul attempts to reduce this to 2 (it merges pre-peephole, the two pass 1 runs, and pass 2, while adding some extra code in the individual procedures to ensure the optimiser doesn't perform worse than before under -O1). A few other developers spotted problems, mostly on Linux, but I believe those have been resolved now.</div><div><br>
</div><div>Admittedly, the bounty that was offered a few months ago is a minor incentive too, although I play around with FPC because I love it!<br>
</div><div><br>
</div><div>I hope despite us being a little bit different in philosophies, we can find common ground and both improve FPC as a team. Thanks for your understanding response, and I hope I wasn't too challenging or critical over #34679.<br>
<br>
Gareth aka. Kit<br>
</div><br>
<br>
<br>
<span style="font-weight: bold;">On Sat 09/02/19 11:32 , Jonas Maebe jonas@freepascal.org sent:<br>
</span><blockquote style="BORDER-LEFT: #F5F5F5 2px solid; MARGIN-LEFT: 5px; MARGIN-RIGHT: 0px; PADDING-LEFT: 5px; PADDING-RIGHT: 0px">On 09/02/19 08:33, J. Gareth Moreton wrote:
<br>
<br>
<span style="color: rgb(102, 102, 102);">> In the last patch that Jonas almost immediately closed, the speed
</span><br>
<span style="color: rgb(102, 102, 102);">> savings were inconclusive because the number of cycles saved is probably
</span><br>
<span style="color: rgb(102, 102, 102);">> only a few dozen, but I would argue it makes the code a bit more
</span><br>
<span style="color: rgb(102, 102, 102);">> reasonable too because it replaces things like loc[low(loc)] with loc[0]
</span><br>
<span style="color: rgb(102, 102, 102);">> and fillchar with a for-loop that initialises each element of an array
</span><br>
<span style="color: rgb(102, 102, 102);">> to zero (it's slightly faster because the element size is a multiple of
</span><br>
<span style="color: rgb(102, 102, 102);">> 16, while fillchar is general-purpose and spends a lot of time jumping
</span><br>
<span style="color: rgb(102, 102, 102);">> around and even performing a multiplication before it starts filling
</span><br>
<span style="color: rgb(102, 102, 102);">> things up). I guess seeing it marked as "won't fix" within 20 minutes
</span><br>
<span style="color: rgb(102, 102, 102);">> was a moment of horror.
</span><br>
<br>
When it comes to the compiler, I'm indeed usually inclined to the "if it
<br>
ain't broken, don't fix it" school of thought. There are already plenty
<br>
of things that break when necessary changes are performed, so why risk
<br>
more breakage by minor touch-ups? As an aside, I also disagree that
<br>
loc[low(loc)] is unreasonable. I consider it defensive programming. That
<br>
said, since the convention is far from followed throughout all of that
<br>
code, removing it doesn't make things really worse either.
<br>
<br>
That said, it is true that the parameter classification code for x86-64
<br>
is quite complex and probably takes a lot of time. Additionally,
<br>
sometimes we even calculate the parameter layout for the same type two
<br>
times in a row (first in push_addr_param, and then again if that one
<br>
returned false).
<br>
<br>
I think a better way to tackle this would be to add a small cache
<br>
(possibly just an array of 10 elements that is searched linearly; a hash
<br>
table risks spending more time and memory/cache flushing on maintaining
<br>
the table than gaining from it) that keeps track of recently requested
<br>
parameter layouts. This would have a potentially much bigger impact, and
<br>
at the same time would not require fiddling with the internals of the
<br>
existing code.
<br>
<br>
<span style="color: rgb(102, 102, 102);">> I just want to avoid working on something, building passion for it which
</span><br>
<span style="color: rgb(102, 102, 102);">> is an unfortunate character flaw of mine, and then seeing it rejected or
</span><br>
<span style="color: rgb(102, 102, 102);">> ignored when it's finished.
</span><br>
<br>
I understand this is annoying. I realise that I am rather conservative
<br>
in this respect. While I think I have good reasons for this stance
<br>
(having spent a lot of time on fixing bugs added by other people while I
<br>
wanted to work on my own stuff; not that other people have never been
<br>
blocked by bugs I added, of course), it also bothers me that I
<br>
discourage new contributors with this attitude.
<br>
<br>
<span style="color: rgb(102, 102, 102);">> I've just hit a couple of impasses since I'm waiting on the node dump
</span><br>
<span style="color: rgb(102, 102, 102);">> feature to be approved and the status of the optimiser overhaul (I tend
</span><br>
<span style="color: rgb(102, 102, 102);">> to be quite good at doing peephole optimisations... at least a peephole
</span><br>
<span style="color: rgb(102, 102, 102);">> optimisation was the very first piece of code I submitted to FPC).
</span><br>
<br>
That's also how I started on FPC in 1997 :) But yes, it definitely was
<br>
much easier back then to jump right in, because there were way fewer
<br>
barriers to getting your code included. At the same time, we paid a
<br>
hefty price for that for many years after that (from copyright issues to
<br>
cleaning out hacks).
<br>
<br>
<br>
Jonas
<br>
_______________________________________________
<br>
fpc-devel maillist - <a href="mailto:fpc-devel@lists.freepascal.org">fpc-devel@lists.freepascal.org</a>
<br>
<a target="_blank" href="<a href="http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel">http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel</a>"><span style="color: red;">http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel</span></a>
<br>
<br>
<br>
</blockquote></HTML>