Add support for adding an external and a tagged pointer

Add support for adding an external and a tagged pointer

This will be used to compute the base pointer of the new unified
representation for both on-heap and external typed arrays. The idea is
that either the external or the tagged pointer is 0 (although in
practice, if the tagged pointer is non-0, the external pointer will
contain the offset from the start of the on-heap typed array to the data
in the on-heap typed array).

The HAdd is marked as depending on new-space promotion, as the tagged
pointer might move during GC, and so the result of the addition needs to
be recomputed.

BUG=v8:3996
R=jarin@chromium.org
LOG=n

Committed: https://crrev.com/4e263bc581081c1fa925554943693f3e386fc815
Cr-Commit-Position: refs/heads/master@{#29760}

[Jarin, 2015-07-20 08:51:28]

jarin@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Jarin, 2015-07-20 08:52:32]

Scary stuff. It looks ok, but fairly brittle. Adding mstarzinger@ to have a
look.

https://codereview.chromium.org/1244693002/diff/20001/src/hydrogen-instructio...
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/1244693002/diff/20001/src/hydrogen-instructio...
src/hydrogen-instructions.h:4890: right->representation().IsTagged()) {
This looks quite brittle, perhaps we should introduce some other flag that
differentiates the special HAdd from ordinary ones.

You could then use the flag to do the right thing in
HAdd::RequiredInputRepresentation.

[Michael Starzinger, 2015-07-20 08:56:38]

https://codereview.chromium.org/1244693002/diff/20001/src/hydrogen-instructio...
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/1244693002/diff/20001/src/hydrogen-instructio...
src/hydrogen-instructions.h:4890: right->representation().IsTagged()) {
On 2015/07/20 08:52:32, jarin wrote:
> This looks quite brittle, perhaps we should introduce some other flag that
> differentiates the special HAdd from ordinary ones.
> 
> You could then use the flag to do the right thing in
> HAdd::RequiredInputRepresentation.

+1

[jochen (gone - plz use gerrit), 2015-07-20 09:37:54]

done, ptal

[Jarin, 2015-07-20 11:23:58]

lgtm.

Even though it would be even better if 

- all the DCHECKs took into account the new flag.
- the flag was explicitly passed on the HAdd constructor (ideally as an enum,
with the default value being the normal add) and then DCHECK that the
representations are in sync with the constructor flag. I just want to avoid the
situation that somebody uses the dangerous HAdd by accident.

[Jarin, 2015-07-20 14:17:56]

Looks even better, I still have two suggestions, but it is up to you.

https://codereview.chromium.org/1244693002/diff/80001/src/hydrogen-instructio...
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/1244693002/diff/80001/src/hydrogen-instructio...
src/hydrogen-instructions.h:4918: }
Hmm, I was imagining you would drive it the other way:

if (external_add_type == AddOfExternalAndTagged) {
  DCHECK(left->representation().IsExternal());
  DCHECK(right->representation().IsTagged());
  SetDependsOnFlag(kNewSpacePromotion);
} else {
  ...
}

https://codereview.chromium.org/1244693002/diff/80001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

https://codereview.chromium.org/1244693002/diff/80001/src/ia32/lithium-ia32.c...
src/ia32/lithium-ia32.cc:1636: instr->right()->representation().IsTagged()));
Maybe you want to factor out the two DCHECK into a separate method, up to you.

Something like 

bool HAdd::IsConsistentExternalRepresentation() {
  return  left()->representation().IsExternal() && 
         ((external_add_type() == AddOfExternalAndInt32 &&
          right()->representation().IsInteger32()) ||
         (external_add_type() == AddOfExternalAndTagged &&
          right()->representation().IsTagged()));
}

[jochen (gone - plz use gerrit), 2015-07-20 14:35:06]

ptal

[Jarin, 2015-07-20 14:51:24]

looks good with one final nit

https://codereview.chromium.org/1244693002/diff/120001/src/mips/lithium-mips.cc
File src/mips/lithium-mips.cc (right):

https://codereview.chromium.org/1244693002/diff/120001/src/mips/lithium-mips....
src/mips/lithium-mips.cc:1625:
DCHECK(instr->left()->representation().IsExternal());
You can leave out DCHECK(instr->left()->representation().IsExternal()), it is
covered by IsConsistentExternalRepresentation.

[jochen (gone - plz use gerrit), 2015-07-20 14:53:29]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2015-07-20 14:53:29]

The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/1244693002/#ps140001
(title: "updates")

[commit-bot: I haz the power, 2015-07-20 14:53:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1244693002/140001

[commit-bot: I haz the power, 2015-07-20 15:17:56]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-07-20 15:18:10]

Patchset 8 (id:??) landed as
https://crrev.com/4e263bc581081c1fa925554943693f3e386fc815
Cr-Commit-Position: refs/heads/master@{#29760}