1229338 - Rename IonOptimizationInfos

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1. Use an enum class for OptimizationLevel

This replaces the OptimizationLevel enum by an enum class. I think the tendency is to use more and more enum classes as they are typed checked by the compiler. However, these enum's (and other ones in the engine) values are used as array indices, so they need an explicit coercion if they are enum classes. I wonder if we really do want to change them to enum classes, in that case? The coercion doesn't cancel the effect of the type checking, as it's done on a known type checked enum class value.

Waldo, what do you think?

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2. Renaming

Instead of OptimizationInfos, this renames this struct to OptimizationLevelInfo, as it is just a smart mapping of OptimizationLevel to OptimizationInfo. I am not too sure about the quality of this name, so I'll take any better alternative.

It also alphabetically reorders the assignments in initInfo/initAsmJS and removes the useless regalloc assignment in initAsmJS (from the time where Ion used the LSRA and AsmJS used the BT regalloc).

Comment 3

[Emanuel Hoogeveen [:ehoogeveen]]

Comment on attachment 8694150 [details] [diff] [review]
1. Use an enum class for OptimizationLevel

I had two comments based on my conversion of AllocKind, but looking at the uses I don't think they're relevant here. Still, they might be good to keep in mind:

1) GCC had a bug where directly manipulating values of an enum class with a type specifier could generate incorrect code: see bug 1143966. The bug was fixed in GCC 4.9.3 and uplifted to GCC 4.8.5, but I don't think we can rely on people using those. I don't think it's relevant here, since you're only casting the values to set the size of a few arrays.

2) If you want to access elements of an array using enum values as indices, you can use EnumeratedArray from MFBT (this also does bound checking). Since you're just using the enum values to set the *size* of a few arrays, I don't think it's necessarily relevant - and would probably be awkward without also switching to range-based iteration (EnumeratedRange).

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #3)
> Comment on attachment 8694150 [details] [diff] [review]
> 1. Use an enum class for OptimizationLevel
> 
> I had two comments based on my conversion of AllocKind, but looking at the
> uses I don't think they're relevant here. Still, they might be good to keep
> in mind:
> 
> 1) GCC had a bug where directly manipulating values of an enum class with a
> type specifier could generate incorrect code: see bug 1143966. The bug was
> fixed in GCC 4.9.3 and uplifted to GCC 4.8.5, but I don't think we can rely
> on people using those. I don't think it's relevant here, since you're only
> casting the values to set the size of a few arrays.
> 
> 2) If you want to access elements of an array using enum values as indices,
> you can use EnumeratedArray from MFBT (this also does bound checking). Since
> you're just using the enum values to set the *size* of a few arrays, I don't
> think it's necessarily relevant - and would probably be awkward without also
> switching to range-based iteration (EnumeratedRange).

Thank you! I wasn't aware about the compiler bug, thanks for the heads up. Also, using an EnumeratedArray would work here as well. Letting Waldo decide what he does prefer.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8694150 [details] [diff] [review]
1. Use an enum class for OptimizationLevel

Review of attachment 8694150 [details] [diff] [review]:
-----------------------------------------------------------------

> these enum's (and other ones in the engine) values are used as array indices, so they need
> an explicit coercion if they are enum classes. I wonder if we really do want to change
> them to enum classes, in that case?

I'm pretty sure, at a brief glance, the answer to this conundrum is to use mozilla::EnumeratedArray, designed for this exact purpose.  :-)

::: js/src/jit/IonOptimizationLevels.cpp
@@ +110,5 @@
>  
>  OptimizationInfos::OptimizationInfos()
>  {
> +    infos_[uint8_t(OptimizationLevel::Normal) - 1].initNormalOptimizationInfo();
> +    infos_[uint8_t(OptimizationLevel::AsmJS) - 1].initAsmjsOptimizationInfo();

This subtraction-y thing is a little strange.  It would be nice to have a comment by |infos_| explaining the ordering/progression of the individual values and what they mean -- I'd have somewhat figured Normal would mean Normal, and so on.

::: js/src/jit/IonOptimizationLevels.h
@@ +273,5 @@
>  
>      const OptimizationInfo* get(OptimizationLevel level) const {
> +        MOZ_ASSERT(level < OptimizationLevel::Count);
> +        MOZ_ASSERT(level != OptimizationLevel::DontCompile);
> +        return &infos_[uint8_t(level) - 1];

...is it really the case that *every* index into this array is offset by one?  It really almost looks like it to me.  That seems unduly confusing, rather than just having (it would appear) one unused entry in the array.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 8694152 [details] [diff] [review]
2. Renaming

Review of attachment 8694152 [details] [diff] [review]:
-----------------------------------------------------------------

Good rename.

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #3)
> I had two comments based on my conversion of AllocKind, but looking at the
> uses I don't think they're relevant here. Still, they might be good to keep
> in mind:

Oh hey, ehoogeveen got here before I did.  :-)  Let's see...

> 2) If you want to access elements of an array using enum values as indices,
> you can use EnumeratedArray from MFBT (this also does bound checking). Since
> you're just using the enum values to set the *size* of a few arrays, I don't
> think it's necessarily relevant - and would probably be awkward without also
> switching to range-based iteration (EnumeratedRange).

Maybe/probably I'm just really confused, but I didn't see anything in the code that would make EnumeratedArray not so relevant.  So this comment confuses me.  :-)

Comment 7

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
> Maybe/probably I'm just really confused, but I didn't see anything in the
> code that would make EnumeratedArray not so relevant.  So this comment
> confuses me.  :-)

Yes, I think *I* was confused, looking at the code you quoted.

Comment 8

[Jeff Walden [:Waldo]]

...also, if the off-by-one fixing requires too much surgery, of course defer to a followup patch/bug as needed.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1. Use an enum class for OptimizationLevel

Yay! EnumeratedArray is exactly what we need here (and before even starting to use it, i've propagated its usage, see also bug 1229642 comment 4 :-)).

So I think the off-by-one comes from the fact that DontCompile doesn't have its own optimization info, and there's a nice progression that DontCompile < Normal. However, for such a small consequence, I've moved DontCompile after Count, so that it doesn't get storage in the array. Though this breaks the order thing, but it doesn't feel too bad (jit tests don't seem to complain); I am just worried that this might affect performance, if there are places outside the .h/.cpp files that are relying on this ordering. For this, f? hannes.

Other than that, using an EnumeratedArray perfectly fits.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 8695454 [details] [diff] [review]
1. Use an enum class for OptimizationLevel

Review of attachment 8695454 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/Ion.cpp
@@ +2107,5 @@
>      TraceLoggerEvent event(logger, TraceLogger_AnnotateScripts, script);
>      AutoTraceLog logScript(logger, event);
>      AutoTraceLog logCompile(logger, TraceLogger_IonCompilation);
>  
> +    MOZ_ASSERT(optimizationLevel != OptimizationLevel::DontCompile);

Should this also assert-not-Count, too?  I realize it let Count happen before, but that looks possibly buggy.  Feel free to do in a separate patch/rev if it simplifies landing this change.

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10)
> Comment on attachment 8695454 [details] [diff] [review]
> 1. Use an enum class for OptimizationLevel
> 
> Review of attachment 8695454 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/Ion.cpp
> @@ +2107,5 @@
> >      TraceLoggerEvent event(logger, TraceLogger_AnnotateScripts, script);
> >      AutoTraceLog logScript(logger, event);
> >      AutoTraceLog logCompile(logger, TraceLogger_IonCompilation);
> >  
> > +    MOZ_ASSERT(optimizationLevel != OptimizationLevel::DontCompile);
> 
> Should this also assert-not-Count, too?  I realize it let Count happen
> before, but that looks possibly buggy.  Feel free to do in a separate
> patch/rev if it simplifies landing this change.

Good point; these assertions can actually go away, as the use of optimizationLevel is to call get(optimizationLevel), which will do the equivalent two assertions (< Count, which implies != DontCompile as well). Pushing. Thanks for the review!

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb630918efd6
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a5ae2ad1b3d

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eb630918efd6
https://hg.mozilla.org/mozilla-central/rev/3a5ae2ad1b3d