1691883 - Fix operandTruncateKind check in canTruncate

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, task, P1)

Description

[Iain Ireland [:iain]]

From nbp's comment here:

The loop within canTruncate is buggy, we should not be reading the operands of an instruction to decide whether to truncate or not the current instruction. The truncation mechanism is a bottom-up propagation of flags, checking the inputs for their truncate kind is non-sense as these would not be refined by the time they are queried. If an instruction is set to TruncateAfterBailout, then we should have even more reasons to truncate the instruction which are following it, as these instructions would already be safe.

ComputeRequestedTruncateKind would probably be the best place to check the hasEagerTruncation flag and replace truncateAfterBailout by NoTruncate when the flag is detected to inhibit Range Analysis even further.

The operandTruncateKind code in canTruncate was motivated by a fuzzbug. A reduced version of the testcase:

function foo(y) {
    return (1 >>> y >> 12) % 1.5 >>> 0
}

with ({}) {}
for (var i = 0; i < 100; i++) {
    foo(1);
}

The key MIR instructions are:

9  rsh ursh7:Int32 constant8:Int32         [Int32]
10 constant 1.5                            [Double]
11 todouble rsh9:Int32                     [Double]
12 mod todouble11:Double constant10:Double [Double]
14 truncatetoint32 mod12:Double            [Int32]

Range analysis determines that the range of rsh9 is I[0, 0], which in turn means that mod12 must also be 0. Because the range is completely precise, Range::optimize determines that the range can't have a fractional part, so canHaveRoundingErrors() is false. ComputeTruncateKind therefore returns TruncateKind::Truncate (because the only use of mod12 is the truncatetoint32), and we truncate mod12 to Int32.

However, one of the operands of the mod is a constant double. In AdjustTruncatedInputs, because MMod::operandTruncateKind always returns TruncateKind::TruncateAfterBailouts, we create a fallible MNumberToInt32 to convert constant 1.5 to Int32. This, obviously, does not go well. We bail out, recompile, and bail out again, triggering an assertion. (If this bailout wasn't marked as EagerTruncation, we would instead have triggered the assertion for recompiling with the same CacheIR.)

In bug 1683535, I added code to check operandTruncateKind before truncating the node. Note that (contrary to what the name implies) this does not check the truncate kind of the inputs. However, some of the implementations (MPhi, MSub/MAdd/MMul, etc...) do use truncateKind() for the node itself. (MDiv and MMod, which motivated this change, don't depend on any state.)

Unfortunately, as nbp points out, truncateKind() for the node itself isn't set until later. (Specifically, it's set as a side effect of calling iter->needTruncation(kind) here, which is somewhat unintuitive.)

I'm not sure what the best fix is here. Converting TruncateAfterBailout to NoTruncate in ComputeRequestedTruncateKind if the flag is set doesn't help. The MMod node has TruncateKind::Truncate. We don't see the TruncateAfterBailout until we call operandTruncateKind, which is wrong in ComputeRequestedTruncateKind for the same reason it is wrong in canTruncate.

nbp: Thoughts? It seems gross to add a new maximumOperandTruncateKind virtual method (which would return the static upper limit on operandTruncateKind), but that's the best I've come up with so far.

Comment 1

[Nicolas B. Pierron [:nbp]]

operandTruncateKind is already supposed to provide the maximal truncation possible given the truncated operation after MDefinition::truncate() is called.

Range analysis is divided in 2 phases (more, but we do not care for this explanation). One phase which is a top-down phase, where the ranges are computed in the direction of the data-flow. A second phase which truncate in a bottom-up phase which is walking the data-flow backward from the usage.

What seems to be missing is a way to prevent the truncation of MMod, using the range analysis information.

I would suggest looking at the needTruncation function as a way to filter out this case by checking if the input range of the right-hand-side has a fractional part. needTruncation would probably be miss-named after this change.

Comment 2

[Iain Ireland [:iain]]

Attachment: Bug 1691883: Check for specific instructions in canTruncate r=jandem,nbp

I originally added the operandTruncateKind check in canTruncate because MMod and MDiv will always force bailouts for their operands. This runs into problems, because some other implementations of operandTruncateKind rely on state that is not set until later. (Some ops call truncateKind(), which is set by the needsTruncation() implementation for that op. MCompare uses truncateOperands_, which is set by MCompare::truncate.

After several attempts, I think the simplest option is to check specifically for MDiv and MMod in canTruncate. These are the only two instructions that can return TruncateAfterBailouts from operandTruncateKind without using TruncateAfterBailouts themselves.

I've also reduced the fuzz test somewhat.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac4b0119506a
Check for specific instructions in canTruncate r=jandem,nbp

Comment 4

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/ac4b0119506a