The plan here is as follows: (1) Replace the js::Value in MConstant with a custom union. Note that we don't need the Value's type tag, because we have the MIRType. (2) Add int64_t to this union. Luke said we don't want NaN canonicalization for wasm, so (1) will take care of that as well.

(I'd ask Hannes to review but he's on PTO this week..) Currently MDefinition has the following methods: bool isConstantValue() const; const Value& constantValue(); const Value* constantVp(); bool constantToBoolean(); These methods can detect/handle MConstant or MBox(MConstant). We want to get rid of the Value dependency here, so this patch changes this to: inline MConstant* maybeConstantValue(); Returning the MConstant* has some benefits: right now we have branches in both isConstantValue and then in constantValue, by returning the MConstant we only need a null check. Furthermore, in a lot of cases it's clear from the context that we don't care about the MBox(MConstant) scenario, for instance: * We're in Odin-only code. * We check def->type() == MIRType_Int32 or something (always false for MBox). * We're in foldsTo or something and the type policy ensures we have an unboxed type. So for those cases I used isConstant/toConstant instead. It'd be great if I could land this soonish, to prevent bitrot.

This patch refactors MConstant::valueToBoolean to not use js::ToBoolean. valueToBoolean should only be called if the type is not MIRType_Magic*. Instead of having all callers check for this, I changed it to use an outparam for the result and return false if the type is not convertible to boolean. I added valueToBooleanInfallible when the caller knows it's not a magic value.

Comment on attachment 8717874 [details] [diff] [review] Part 1 - Refactor constantValue and friends Review of attachment 8717874 [details] [diff] [review]: ----------------------------------------------------------------- So this looks safe, assuming that: - IonBuilder doesn't create MBox-es (dxr seems to confirm this). - specialization_ == T => type() == T If that's not true, a few changes are unsafe. Otherwise, r=me (remarks are just stylistic nits and questions to ensure I've understood what the assumptions are). Thanks for this patch! ::: js/src/jit/AlignmentMaskAnalysis.cpp @@ +44,5 @@ > return; > > MDefinition* lhs = ptr->toBitAnd()->getOperand(0); > MDefinition* rhs = ptr->toBitAnd()->getOperand(1); > + if (lhs->isConstant()) Can you add a MOZ_ASSERT at the top of this function that we're compiling asm.js? There is something like IsCompilingAsmJS(). That is just to make sure we don't miss an optimization if we were to enable this for Ion. ::: js/src/jit/IonBuilder.cpp @@ +3664,5 @@ > return improveTypesAtNullOrUndefinedCompare(ins, trueBranch, test); > } > > if ((ins->lhs()->isTypeOf() || ins->rhs()->isTypeOf()) && > + (ins->lhs()->isConstant() || ins->rhs()->isConstant())) This doesn't make us miss any optimizations because at the time of IonBuilding, there are no boxes yet, right? @@ +9486,3 @@ > JSObject* tarr = nullptr; > + if (obj->maybeConstantValue() && obj->maybeConstantValue()->type() == MIRType_Object) > + tarr = &obj->maybeConstantValue()->value().toObject(); Could store the result of ->maybeConstantValue() in a temp variable to avoid redundancy. @@ +9544,5 @@ > > // If the index is an already shifted constant, undo the shift to get the > // absolute offset being accessed. > + if (id->maybeConstantValue() && id->maybeConstantValue()->type() == MIRType_Int32) { > + int32_t index = id->maybeConstantValue()->value().toInt32(); ditto @@ +9552,5 @@ > } > > if (!id->isRsh() || id->isEffectful()) > return nullptr; > + if (!id->getOperand(1)->maybeConstantValue()) ditto @@ +13390,5 @@ > > MOZ_ASSERT(!*emitted); > > jsid propId; > + if (!id->maybeConstantValue() || !ValueToIdPure(id->maybeConstantValue()->value(), &propId)) ditto ::: js/src/jit/Lowering.cpp @@ +715,5 @@ > MOZ_ASSERT(opd->type() != MIRType_String); > > // Testing a constant. > + if (opd->maybeConstantValue() && !opd->maybeConstantValue()->value().isMagic()) { > + bool result = opd->maybeConstantValue()->valueToBoolean(); ditto ::: js/src/jit/MCallOptimize.cpp @@ +2633,5 @@ > > MOZ_ASSERT(secondArg->type() == MIRType_Boolean); > + MOZ_ASSERT(secondArg->isConstant()); > + > + bool mustBeFloat32 = secondArg->toConstant()->value().toBoolean(); You can use valueToBoolean() here. @@ +2663,5 @@ > > MOZ_ASSERT(secondArg->type() == MIRType_Boolean); > + MOZ_ASSERT(secondArg->isConstant()); > + > + bool mustBeRecovered = secondArg->toConstant()->value().toBoolean(); And here too. ::: js/src/jit/MIR.cpp @@ +81,2 @@ > static MConstant* > EvaluateConstantOperands(TempAllocator& alloc, MBinaryInstruction* ins, bool* ptypeChange = nullptr) This is called in foldsTo, so where types are definite, indirectly called in MBinaryArithInstruction::setNumberSpecialization, which is called at IonBuilder, so I assume this is safe to assume no boxes since we're still in IonBuilder, is that right? @@ +368,5 @@ > return MTest::New(alloc, opop->toNot()->input(), ifTrue(), ifFalse()); > return MTest::New(alloc, op->toNot()->input(), ifFalse(), ifTrue()); > } > > + MConstant* opConst = op->maybeConstantValue(); We're in a foldsTo function, so we can just use isConstant()/toConstant() here. @@ +2747,5 @@ > if (specialization_ != MIRType_Int32) > return; > > // Try removing divide by zero check > + if (rhs()->isConstant() && !rhs()->toConstant()->value().isInt32(0)) I think this is safe (we're not losing any optimization opportunities) because specialization_ == MIRType_Int32 implies type() == MIRType_Int32. Can you MOZ_ASSERT this above, please? @@ +3003,5 @@ > if (def->isUrsh()) { > *pwrapped = def->toUrsh()->getOperand(0); > MDefinition* rhs = def->toUrsh()->getOperand(1); > return !def->toUrsh()->bailoutsDisabled() > + && rhs->maybeConstantValue() pre-existing: && go to the end of the previous line @@ +3594,5 @@ > > MDefinition* > MClampToUint8::foldsTo(TempAllocator& alloc) > { > + if (MConstant* inputConst = input()->maybeConstantValue()) { Could be isConstant()/toConstant(), as we're in a foldsTo method? @@ +4050,5 @@ > MDefinition* > MNot::foldsTo(TempAllocator& alloc) > { > // Fold if the input is constant > + if (input()->maybeConstantValue() && !input()->maybeConstantValue()->value().isMagic()) { ditto

This is a large patch, but for the most part pretty mechanical: * It replaces MConstant's value() and vp() methods with toInt32(), toNumber(), etc. * LAllocation can store a Value* pointer, I had to change this to store the MConstant* instead. * I added a toJSValue() method to MConstant that returns a js::Value. This will assert for int64, but that's fine as wasm/Odin code shouldn't call this method. * MConstant still contains a js::Value, but after this patch it should be easy to refactor this as it's no longer accessed outside MConstant. * We'll need to decide how isNumber/toNumber/IsNumberType interact with int64. I'll deal with this later, to not make this patch even bigger.

Oh I forgot to mention: this also turned out nicely for float32: we can now call const->toFloat32() instead of const->value().toDouble().

Comment on attachment 8718258 [details] [diff] [review] Part 3 - Refactor MConstant interface Review of attachment 8718258 [details] [diff] [review]: ----------------------------------------------------------------- I like how you're doing this.

Thanks for the quick reviews! (In reply to Benjamin Bouvier [:bbouvier] from comment #3) > This doesn't make us miss any optimizations because at the time of > IonBuilding, there are no boxes yet, right? Right. Also, the function we're calling (improveTypesAtTypeOfCompare) did toConstant(), so if we could get a box here we'd have had a bug. > You can use valueToBoolean() here. valueToBoolean is a bit slower because it can convert any type to boolean. toBoolean, OTOH, asserts the type is boolean and returns its value directly (maybe valueToBoolean should be renamed convertToBoolean..). > ::: js/src/jit/MIR.cpp > @@ +81,2 @@ > > static MConstant* > > EvaluateConstantOperands(TempAllocator& alloc, MBinaryInstruction* ins, bool* ptypeChange = nullptr) > > This is called in foldsTo, so where types are definite, indirectly called in > MBinaryArithInstruction::setNumberSpecialization, which is called at > IonBuilder, so I assume this is safe to assume no boxes since we're still in > IonBuilder, is that right? Yeah my reasoning here was that we have the following assert, which implies left/right are not boxed: MOZ_ASSERT(IsNumberType(left->type()) && IsNumberType(right->type())); > > + MConstant* opConst = op->maybeConstantValue(); > > We're in a foldsTo function, so we can just use isConstant()/toConstant() > here. MTest can have a Box as input (determined by its type policy). Same for MNot, etc. > I think this is safe (we're not losing any optimization opportunities) > because specialization_ == MIRType_Int32 implies type() == MIRType_Int32. > Can you MOZ_ASSERT this above, please? Good idea.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cbce3cad8b7 https://hg.mozilla.org/integration/mozilla-inbound/rev/dae4acefe53b https://hg.mozilla.org/integration/mozilla-inbound/rev/98bc20ce8452

This patch replaces MConstant's private Value with a custom Payload union. Some places created an MConstant with DoubleValue and then did setResultType(MIRType_Float32). That no longer works with this patch, because floats are no longer stored as doubles. I added MConstant::NewFloat32 to fix this. Other than that, this was pretty straight-forward (the previous patches did the heavy lifting).

Comment on attachment 8718781 [details] [diff] [review] Part 4 - Replace MConstant's Value with custom type Review of attachment 8718781 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/MIR.h @@ +1400,5 @@ > + void setDouble(double d) { data_.asBits = 0; data_.d = d; } > + void setString(JSString* s) { data_.asBits = 0; data_.str = s; } > + void setSymbol(JS::Symbol* s) { data_.asBits = 0; data_.sym = s; } > + void setObject(JSObject* o) { data_.asBits = 0; data_.obj = o; } > + } payload_; Since this is a private type of MConstant and it's not providing much in the way of assertions, I'd make Payload be the union (no methods) and initialize the union members directly.

https://hg.mozilla.org/mozilla-central/rev/6cbce3cad8b7 https://hg.mozilla.org/mozilla-central/rev/dae4acefe53b https://hg.mozilla.org/mozilla-central/rev/98bc20ce8452

(In reply to Luke Wagner [:luke] from comment #10) > Since this is a private type of MConstant and it's not providing much in the > way of assertions, I'd make Payload be the union (no methods) and initialize > the union members directly. The idea is that the Payload type has setter methods that ensure all payload bits are initialized. For instance, if you do |payload_.i32 = i| and leave the upper bits uninitialized, the equals() and hashValue() methods no longer work as expected. If you think protecting against this is overkill, I can remove the separate type though.

This patch adds: * MIRType_Int64 * IsTypeRepresentableAsDouble(type): this function returns true for int32/float32/double. Initially I also wanted to add a method to MConstant that'd return true for small int64 values, but I now think making this purely type based is less bug prone. We can handle MIRType_Int64 in more places later. * MConstant::NewInt64(alloc, int64_t) returns an int64 MConstant.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d170a9874c2b https://hg.mozilla.org/integration/mozilla-inbound/rev/1493196bb7a0

Comment on attachment 8719713 [details] [diff] [review] Part 5 - Support int64 constants Review of attachment 8719713 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/MIR.h @@ +1500,2 @@ > } > double toNumber() const { It seems like there is a mismatch between "Double" and "Number" and one should be changed to match the other. Since toDouble is already assigned above, perhaps then isTypeRepresentableAsNumber()?

(In reply to Luke Wagner [:luke] from comment #15) > It seems like there is a mismatch between "Double" and "Number" and one > should be changed to match the other. Since toDouble is already assigned > above, perhaps then isTypeRepresentableAsNumber()? Hm that's a bit confusing because int64 is also a "number" (see for instance IsNumberType). What if I rename toNumber to numberAsDouble or something?

(In reply to Jan de Mooij [:jandem] from comment #16) > Hm that's a bit confusing because int64 is also a "number" (see for instance > IsNumberType). What if I rename toNumber to numberAsDouble or something? Bah, I'm used to needinfo'ing myself.

If we take "number" to be the JS definition of "number" which is actually a ieee754 double, then I think the name is valid. isTypeRepresentableAsJSNumber could make this more clear but it's a big ugly... numberAsDouble is a good idea regardless since it makes it more clear that we're not just getting the payload (like the other toX methods). The only problem is we usually use "as" to mean "asserted downcast" and "to" to mean "perform some operation", so "to" I think is the better connective. What about numberToDouble?

numberToDouble is fine. Thanks!

https://hg.mozilla.org/mozilla-central/rev/d170a9874c2b https://hg.mozilla.org/mozilla-central/rev/1493196bb7a0