On 2014/03/26 15:49:40, Michael Starzinger wrote:
I am not yet entirely convinced that this change actually improves the
situation
overall. Admittedly it removes one of the "if (do_something_different)
{ ...
}"
branches in the lithium code generator. But that comes at the expense of
duplicating 40 lines of code in each code generator. And they are
separated
far
enough that I can totally imagine my stupid-future-self updating one
instance
but not the other.
This actually improves the situation *a lot*: With this CL things are now
highly
consistent across platform. We have 3 division-like operations on the
Hydrogen
level, which are consistently lowered to 3 Lithium variants each (power of 2
constant, other constant, general case) with this CL. Without that CL, 3
platforms behave differently, trying so save a few dozen lines of simple
code
(yes, partly duplicated code, but that's a *totally* different story). So in
effect we're trading some tiny simplification for various "WTF?" moments
when
reading the code, which is quite bad IMHO.
I do +1 the part that introduces a separate LInstruction for the flooring
division, this makes the distinction in semantics clear in the lithium
graph.
:-)
I would be totally fine with this change if the common code in the
generator
would still be shared. For example by using a helper function or support
from
the macro assembler, as you mentioned yourself in the CL description.
Perhaps my initial comment about this CL was not clear enough: I totally
agree
the common CS folklore that redundancy is the root of all evil, but in our
case
fixing things is a 2-step process: First we have to get the structure right
(this CL), and then (and only then) see what can be abstracted out. We
literally
have *thousands* of lines of copy-n-paste code in each backend, and I'm
totally
willing to clean a few things related to this CL up in a follow-up CL. But
this
will very probably touch non LFlooringDiv*-related stuff and therefore would
totally hide the intent and simplicity of this CL at hand.
In a nutshell: I really, really like to land this CL, we (= I? :-) can then
have
a *global* look at the backends to see what can be abstracted out. I hate
our
approach of fixing/"improving" things only locally, which normally leaves a
bigger mess behind than before.
https://codereview.chromium.org/212703002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
