[strong] Refactor ObjectStrength into a replacement for strong boolean args

[strong] Refactor ObjectStrength into a replacement for strong boolean args

Boolean "is_strong" parameters have begun to proliferate across areas where
strong mode semantics are different. This CL repurposes the existing
ObjectStrength enum as a replacement for them.

BUG=v8:3956
LOG=N

Committed: https://crrev.com/dd8544495109804c632bfbcefc3c5c701e0a8c43
Cr-Commit-Position: refs/heads/master@{#28839}

[conradw, 2015-06-03 19:32:46]

conradw@chromium.org changed reviewers:
+ arv@chromium.org, rossberg@chromium.org

[conradw, 2015-06-03 19:32:48]

PTAL

[arv (Not doing code reviews), 2015-06-04 15:24:30]

Is there a reason we need both LanguageMode and Strength? Can we just user
LanguageMode in the places where we use Strength?

https://codereview.chromium.org/1144183004/diff/180001/src/compiler/js-operat...
File src/compiler/js-operator.h (right):

https://codereview.chromium.org/1144183004/diff/180001/src/compiler/js-operat...
src/compiler/js-operator.h:328: const Operator* LessThan(Strength strength);
I find the old code cleaner. LanguageMode is more descriptive than Strength.

[adamk, 2015-06-04 16:20:56]

On 2015/06/04 15:24:30, arv wrote:
> Is there a reason we need both LanguageMode and Strength? Can we just user
> LanguageMode in the places where we use Strength?

+1 to this. From outside the strong mode work, needing to understand that
Strength::FIRM means the same thing as STRONG seems unfortunate.

[conradw, 2015-06-04 18:38:31]

On 2015/06/04 16:20:56, adamk wrote:
> On 2015/06/04 15:24:30, arv wrote:
> > Is there a reason we need both LanguageMode and Strength? Can we just user
> > LanguageMode in the places where we use Strength?
> 
> +1 to this. From outside the strong mode work, needing to understand that
> Strength::FIRM means the same thing as STRONG seems unfortunate.

Code stubs are cached according to their key, one part of which is the minor key
(computed from the IC state), which must now contain some indication of language
mode for those stubs that have different strong mode behaviour. However, if the
full language mode was included in the key, this would mean that sloppy and
strict versions of the stub would be cached separately, a regression.

The solution was to store some sort of "is_strong" bool in the key. However,
this meant that when an IC transitioned and a new stub needed to be
compiled/retrieved, none of the involved functions had access to the full
language mode any more, just this bool. This lead to a lot of functions getting
a boolean argument tacked on instead of a more descriptive language mode
parameter, and has also caused a few implicit conversion issues.

One other solution was to have the states for each IC lie about their language
mode, returning "sloppy" even in strict mode, but this was shot down since it
had the potential to cause unintuitive mismatches down the road. Without "lying"
in this way, there's no obvious way for the functions changed in this CL to talk
about language mode, they will have to take some kind of two-valued parameter,
which used to be a bool, and in this CL becomes a strongly typed enum.

One nitpicky destinction between Strength::FIRM and STRONG is that FIRM is also
used to signal strong object behaviour instead of just behaviour affected by the
current language mode, although that's something I'm less familiar with since
they're Andreas' changes.

[conradw, 2015-06-04 18:51:40]

On 2015/06/04 18:38:31, conradw wrote:
> On 2015/06/04 16:20:56, adamk wrote:
> > On 2015/06/04 15:24:30, arv wrote:
> > > Is there a reason we need both LanguageMode and Strength? Can we just user
> > > LanguageMode in the places where we use Strength?
> > 
> > +1 to this. From outside the strong mode work, needing to understand that
> > Strength::FIRM means the same thing as STRONG seems unfortunate.
> 
> snip

"FIRM" is a slightly unfortunate name I agree, it could be Strength::STRONG,
which wouldn't cause a clash because it's an enum class, but might be confusing
in its own way just because of the similar name. This is something rossberg and
I discussed and never quite reached a satisfying agreement on. Thoughts?

[adamk, 2015-06-04 20:25:11]

On 2015/06/04 18:38:31, conradw wrote:
> On 2015/06/04 16:20:56, adamk wrote:
> > On 2015/06/04 15:24:30, arv wrote:
> > > Is there a reason we need both LanguageMode and Strength? Can we just user
> > > LanguageMode in the places where we use Strength?
> > 
> > +1 to this. From outside the strong mode work, needing to understand that
> > Strength::FIRM means the same thing as STRONG seems unfortunate.
> 
> Code stubs are cached according to their key, one part of which is the minor
key
> (computed from the IC state), which must now contain some indication of
language
> mode for those stubs that have different strong mode behaviour. However, if
the
> full language mode was included in the key, this would mean that sloppy and
> strict versions of the stub would be cached separately, a regression.
> 
> The solution was to store some sort of "is_strong" bool in the key. However,
> this meant that when an IC transitioned and a new stub needed to be
> compiled/retrieved, none of the involved functions had access to the full
> language mode any more, just this bool. This lead to a lot of functions
getting
> a boolean argument tacked on instead of a more descriptive language mode
> parameter, and has also caused a few implicit conversion issues.

I think you've convinced me that Strength and LanguageMode are different. But
perhaps the solution is to choose a name that more closely reflects the usage.
StubStrength::STRONG vs StubStrength::NORMAL, for example. Just an idea; it's
hard to tell how many things will have to know about Strength, so it's hard for
me to guess whether that naming will become inaccurate at some point.

[conradw, 2015-06-04 20:31:21]

> I think you've convinced me that Strength and LanguageMode are different. But
> perhaps the solution is to choose a name that more closely reflects the usage.
> StubStrength::STRONG vs StubStrength::NORMAL, for example. Just an idea; it's
> hard to tell how many things will have to know about Strength, so it's hard
for
> me to guess whether that naming will become inaccurate at some point.

It definitely seems like a better name is needed. I will have a think, and look
more closely at its use with strong objects. This feedback has leant me much
more towards Andreas' position that FIRM would be better as STRONG, despite the
name overlap.

[conradw, 2015-06-05 08:54:59]

Redid the names, I like Adam's suggestion that non-strong mode should be
"NORMAL", since it doesn't inspire the same confusion that seeing "WEAK"
scattered around in object initialization would.

I can't think of a better name for the enum itself than "Strength", since it's
already being used by strong object code as well.

[conradw, 2015-06-05 09:06:17]

On 2015/06/05 08:54:59, conradw wrote:
> Redid the names, I like Adam's suggestion that non-strong mode should be
> "NORMAL", since it doesn't inspire the same confusion that seeing "WEAK"
> scattered around in object initialization would.
> 
> I can't think of a better name for the enum itself than "Strength", since it's
> already being used by strong object code as well.

Possible alternative, separate ObjectStrength and StubStrength enums.

Seems like it doesn't scale though, and would require two sets of identical
boilerplate in globals.h.

[rossberg, 2015-06-08 09:27:42]

I, too, prefer Strength::STRONG, now that it is an enum class. The only reason I
picked FIRM before was the name collision problem.

> Redid the names, I like Adam's suggestion that non-strong mode should be
> "NORMAL", since it doesn't inspire the same confusion that seeing "WEAK"
> scattered around in object initialization would.

This one I don't like. NORMAL is far too generic to be indicative of anything,
and not an obvious antonym of STRONG. I find it far more confusing (in
particular, since we already have half a dozen enums with NORMAL).

> > I can't think of a better name for the enum itself than "Strength", since
it's
> > already being used by strong object code as well.
> 
> Possible alternative, separate ObjectStrength and StubStrength enums.
> 
> Seems like it doesn't scale though, and would require two sets of identical
> boilerplate in globals.h.

Yes, let's not inflate enums. I think "strength" will show up in a couple more
places in the runtime, and it's a natural partitioning of the language mode
(which eventually will contain TYPED as a 4th value, which would also project to
Strength::STRONG).

[rossberg, 2015-06-08 11:34:21]

https://codereview.chromium.org/1144183004/diff/220001/src/compiler/js-operat...
File src/compiler/js-operator.h (right):

https://codereview.chromium.org/1144183004/diff/220001/src/compiler/js-operat...
src/compiler/js-operator.h:328: const Operator* LessThan(Strength strength);
As discussed offline, on the abstraction level of the TF graph nodes/operators,
it seems preferable to stick to LanguageMode. Only lower to Strength where you
go from syntax to runtime/code.

[conradw, 2015-06-08 11:48:49]

Also NORMAL -> WEAK as per other comment

https://codereview.chromium.org/1144183004/diff/220001/src/compiler/js-operat...
File src/compiler/js-operator.h (right):

https://codereview.chromium.org/1144183004/diff/220001/src/compiler/js-operat...
src/compiler/js-operator.h:328: const Operator* LessThan(Strength strength);
On 2015/06/08 11:34:21, rossberg wrote:
> As discussed offline, on the abstraction level of the TF graph
nodes/operators,
> it seems preferable to stick to LanguageMode. Only lower to Strength where you
> go from syntax to runtime/code.

Done.

[rossberg, 2015-06-08 12:10:29]

lgtm

[conradw, 2015-06-08 12:14:26]

The CQ bit was checked by conradw@chromium.org

[commit-bot: I haz the power, 2015-06-08 12:14:33]

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

[commit-bot: I haz the power, 2015-06-08 12:18:04]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-06-08 12:18:25]

Patchset 13 (id:??) landed as
https://crrev.com/dd8544495109804c632bfbcefc3c5c701e0a8c43
Cr-Commit-Position: refs/heads/master@{#28839}