Add equality type parameter to HCompareObjectAndBranch

Add equality type parameter to HCompareObjectAndBranch

R=titzer@chromium.org

[titzer, 2013-07-29 13:24:41]

https://codereview.chromium.org/21014004/diff/1/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/21014004/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:3076: void
HCompareObjectAndBranch::PrintDataTo(StringStream* stream) {
We should probably print the token now too, right?

https://codereview.chromium.org/21014004/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/21014004/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:4175: Token::Value token = Token::EQ) {
I looked, but I couldn't find any uses with the token == NE. We have the
IfBuilder which can do IfNot, so do we need this parameterization?

[danno, 2013-07-29 14:44:59]

https://codereview.chromium.org/21014004/diff/1/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/21014004/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:3076: void
HCompareObjectAndBranch::PrintDataTo(StringStream* stream) {
On 2013/07/29 13:24:41, titzer wrote:
> We should probably print the token now too, right?

Done.

https://codereview.chromium.org/21014004/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/21014004/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:4175: Token::Value token = Token::EQ) {
On 2013/07/29 13:24:41, titzer wrote:
> I looked, but I couldn't find any uses with the token == NE. We have the
> IfBuilder which can do IfNot, so do we need this parameterization?

It's used in a follow-on CL that I'm preparing, but I wanted to do this
incrementally. The problem with IfNot is that it just switches the true/false
branch blocks. But the instructions that are output are still: "cmp XXXX,
b(eq,XXXX)", which makes the fall-through case always the not-equal case. In the
follow-on CL to convert numeric dictionary lookup to hydrogen that I'm
preparing, in order to generate the same code as the old assembly-language
version, the fall-through case needs to be the "ne" case, and it measurably
affects performance if it isn't because it has to create extra jumps to sort of
the graph topology. Someday, out instruction selection should be able to figure
this out itself, but that's unfortunately a long way off.

[titzer, 2013-07-30 08:31:05]

On 2013/07/29 14:44:59, danno wrote:
> https://codereview.chromium.org/21014004/diff/1/src/hydrogen-instructions.cc
> File src/hydrogen-instructions.cc (right):
> 
>
https://codereview.chromium.org/21014004/diff/1/src/hydrogen-instructions.cc#...
> src/hydrogen-instructions.cc:3076: void
> HCompareObjectAndBranch::PrintDataTo(StringStream* stream) {
> On 2013/07/29 13:24:41, titzer wrote:
> > We should probably print the token now too, right?
> 
> Done.
> 
> https://codereview.chromium.org/21014004/diff/1/src/hydrogen-instructions.h
> File src/hydrogen-instructions.h (right):
> 
>
https://codereview.chromium.org/21014004/diff/1/src/hydrogen-instructions.h#n...
> src/hydrogen-instructions.h:4175: Token::Value token = Token::EQ) {
> On 2013/07/29 13:24:41, titzer wrote:
> > I looked, but I couldn't find any uses with the token == NE. We have the
> > IfBuilder which can do IfNot, so do we need this parameterization?
> 
> It's used in a follow-on CL that I'm preparing, but I wanted to do this
> incrementally. The problem with IfNot is that it just switches the true/false
> branch blocks. But the instructions that are output are still: "cmp XXXX,
> b(eq,XXXX)", which makes the fall-through case always the not-equal case. In
the
> follow-on CL to convert numeric dictionary lookup to hydrogen that I'm
> preparing, in order to generate the same code as the old assembly-language
> version, the fall-through case needs to be the "ne" case, and it measurably
> affects performance if it isn't because it has to create extra jumps to sort
of
> the graph topology. Someday, out instruction selection should be able to
figure
> this out itself, but that's unfortunately a long way off.

That makes sense. But maybe it would be better to make it explicit to the
backends which direction we expect the branch to go (via assume_taken or
assume_not_taken) instead of it implicitly assuming the branch to be not taken
and then using EQ/NE.