[JSC] Introduce TrustedImm32Masked8 for 8bit operations

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[JSC] MacroAssemblerX86::branch8 should accept unsigned 8bit value

[Yusuke Suzuki]

Created attachment 282519 [details]
Patch

[WebKit Commit Bot]

Attachment 282519 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/testb3.cpp:7325:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:7327:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:7327:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 282520 [details]
Patch

[WebKit Commit Bot]

Attachment 282520 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/testb3.cpp:7325:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:7327:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:7327:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 282520 [details]
Patch

Attachment 282520 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1605278

New failing tests:
js/regress/misc-bugs-847389-jpeg2000.html

[Build Bot]

Created attachment 282524 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282520 [details]
Patch

Attachment 282520 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1605277

New failing tests:
js/regress/misc-bugs-847389-jpeg2000.html

[Build Bot]

Created attachment 282525 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282520 [details]
Patch

Attachment 282520 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1605280

New failing tests:
js/regress/misc-bugs-847389-jpeg2000.html

[Build Bot]

Created attachment 282526 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Build Bot]

Comment on attachment 282520 [details]
Patch

Attachment 282520 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1605289

New failing tests:
js/regress/misc-bugs-847389-jpeg2000.html

[Build Bot]

Created attachment 282527 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Yusuke Suzuki]

Created attachment 282528 [details]
Patch

[WebKit Commit Bot]

Attachment 282528 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/testb3.cpp:7325:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:7327:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:7327:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 282529 [details]
Patch

[WebKit Commit Bot]

Attachment 282529 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/testb3.cpp:7325:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:7327:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:7327:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 282529 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=282529&action=review

> Source/JavaScriptCore/ChangeLog:9
> +        So the assertion here should be a little permisive; accepting -128 to 255.

typo: /permisive/permissive/.

[Yusuke Suzuki]

(In reply to comment #17)
> Comment on attachment 282529 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282529&action=review
> 
> > Source/JavaScriptCore/ChangeLog:9
> > +        So the assertion here should be a little permisive; accepting -128 to 255.
> 
> typo: /permisive/permissive/.

Thanks, fixed.

[Yusuke Suzuki]

Created attachment 282562 [details]
Patch

[WebKit Commit Bot]

Attachment 282562 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/testb3.cpp:7325:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:7327:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:7327:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Comment on attachment 282562 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=282562&action=review

Added the comment.

> LayoutTests/TestExpectations:990
> +[ Debug ] js/regress/misc-bugs-847389-jpeg2000.html [ Pass Timeout ]

Unfortunately, this test takes much time in Debug build.
But if this patch is not applied, this test causes crash soon before this test is timed out.

[Mark Lam]

(In reply to comment #21)
> Comment on attachment 282562 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282562&action=review
> 
> Added the comment.
> 
> > LayoutTests/TestExpectations:990
> > +[ Debug ] js/regress/misc-bugs-847389-jpeg2000.html [ Pass Timeout ]
> 
> Unfortunately, this test takes much time in Debug build.
> But if this patch is not applied, this test causes crash soon before this
> test is timed out.

Maybe we should skip this test for the debug build?  We still get coverage with the release build, right?

[Yusuke Suzuki]

(In reply to comment #22)
> (In reply to comment #21)
> > Comment on attachment 282562 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=282562&action=review
> > 
> > Added the comment.
> > 
> > > LayoutTests/TestExpectations:990
> > > +[ Debug ] js/regress/misc-bugs-847389-jpeg2000.html [ Pass Timeout ]
> > 
> > Unfortunately, this test takes much time in Debug build.
> > But if this patch is not applied, this test causes crash soon before this
> > test is timed out.
> 
> Maybe we should skip this test for the debug build?  We still get coverage
> with the release build, right?

Right. One cons is that the previous (incorrect) assertion does not fire with this configuration (since the wrong assertion was not release assrt.) Is it preferable?

[Yusuke Suzuki]

Created attachment 282566 [details]
Patch

Update the expectation to Skip in Debug

[WebKit Commit Bot]

Attachment 282566 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/testb3.cpp:7325:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:7327:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:7327:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

I think that the correct fix is to remove the assertion. We can't prevent Air constant-folding a large immediate into that immediate field, since it's described to Air as simply being 32-bit. I think we want to just be clear that the immediate is cast to int8 before anything else happens. I think that means removing mention of that immediate being not well defined.

[Yusuke Suzuki]

Committed r202797: <http://trac.webkit.org/changeset/202797>

[Yusuke Suzuki]

(In reply to comment #26)
> I think that the correct fix is to remove the assertion. We can't prevent
> Air constant-folding a large immediate into that immediate field, since it's
> described to Air as simply being 32-bit. I think we want to just be clear
> that the immediate is cast to int8 before anything else happens. I think
> that means removing mention of that immediate being not well defined.

Ah, OK, I understand what you mean.
In Air, when we define U:G:8, this means use of the low 8 bits of a general-purpose register / value. So there are no guarantee that the upper bits (64 - 8) are cleared. So, in Air, we cannot guarantee that the given value is exactly 8bit in branch8.

I'll drop the assertion. Make sense.

[Csaba Osztrogonác]

(In reply to comment #27)
> Committed r202797: <http://trac.webkit.org/changeset/202797>

It caused failures on the debug bots:
- https://build.webkit.org/builders/Apple%20El%20Capitan%2032-bit%20JSC%20%28BuildAndTest%29/builds/2819
- https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20JSC%20%28Tests%29/builds/3340
- https://build.webkit.org/builders/Apple%20Yosemite%2032-bit%20JSC%20%28BuildAndTest%29/builds/9668
- Yosemite

[Yusuke Suzuki]

(In reply to comment #29)
> (In reply to comment #27)
> > Committed r202797: <http://trac.webkit.org/changeset/202797>
> 
> It caused failures on the debug bots:
> -
> https://build.webkit.org/builders/Apple%20El%20Capitan%2032-
> bit%20JSC%20%28BuildAndTest%29/builds/2819
> -
> https://build.webkit.org/builders/
> Apple%20El%20Capitan%20Debug%20JSC%20%28Tests%29/builds/3340
> -
> https://build.webkit.org/builders/Apple%20Yosemite%2032-
> bit%20JSC%20%28BuildAndTest%29/builds/9668
> - Yosemite

Thanks. That's timeout. Maybe, in addition to LayoutTests/TestExpectation file, I need to add // @skip etc.... comment to the js/regress/script-tests/* file.
I'll annotate them as skip in debug build (In jsc tests).

[Yusuke Suzuki]

Committed r202805: <http://trac.webkit.org/changeset/202805>

[Yusuke Suzuki]

Reopening to attach new patch.

[Yusuke Suzuki]

Created attachment 282697 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 282697 [details]
Patch

OK, the follow up patch is uploaded. This patch removes the assertions since they are incorrect under the Air.
And to achieve the 8bit cast easily, we introduce new type, TrustedImm32Clamped<int8_t>, and use it for 8bit operations.

[Yusuke Suzuki]

Created attachment 282708 [details]
Patch

[Yusuke Suzuki]

Ping? :D

[Filip Pizlo]

Comment on attachment 282708 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=282708&action=review

> Source/JavaScriptCore/ChangeLog:18
> +        It takes TrustedImm32 and clamps it to the range of int8_t. 8bits operations now take
> +        TrustedImm32Clamped8 as its argument instead of TrustedImm32.

How is this better than adding the casts explicitly and still using TrustedImm32?

Also, "clamped" is the wrong term.  Usually, clamped implies saturation - so 300 would become 127, rather than 44, which is what TrustedImm32Clamped<int8_t> would do.

[Yusuke Suzuki]

Comment on attachment 282708 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=282708&action=review

Thanks!

>> Source/JavaScriptCore/ChangeLog:18
>> +        TrustedImm32Clamped8 as its argument instead of TrustedImm32.
> 
> How is this better than adding the casts explicitly and still using TrustedImm32?
> 
> Also, "clamped" is the wrong term.  Usually, clamped implies saturation - so 300 would become 127, rather than 44, which is what TrustedImm32Clamped<int8_t> would do.

I think `TrustedImm32Masked8 (something like this)` is better since the type TrustedImm32Masked8 and the function signature gives us the context that, "the function takes Imm32, but only 8bit is effective".

[Yusuke Suzuki]

Created attachment 283103 [details]
Patch

Updated the patch. Fix the name, TrustedImm32Clamped8 to TrustedImm32Masked8

[Yusuke Suzuki]

Comment on attachment 283103 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=283103&action=review

Request for review :)
Updated the patch with renaming `TrustedImm32Clamped8` to `TrustedImm32Masked8`.

> Source/JavaScriptCore/ChangeLog:18
> +        TrustedImm32Masked8 as its argument instead of TrustedImm32.

I prefer using TrustedImm32Masked8 to using TrustedImm32 and explicit casting in the method.
Since it provides descriptive type TrustedImm32Masked8 as the function's signature.
This signature gives us more context than using TrustedImm32: we can take 32bit value, but only 8bit is effective in this method.

Another benefit is that TrustedImm32Masked8 can guarantee that the imm value is already masked.
At that time, we don't need to bother whether the given imm is already masked or not, the type explicitly answers the question.
Whether the value is masked or not is important when selecting better instructions. For example, we have several code like,

```
if (!imm.m_value) {
    store8(ARM64Registers::zr, address);
    return;
}
move(imm, getCachedDataTempRegisterIDAndInvalidate());
store8(dataTempRegister, address);
```

If the imm is 0, we can leverage the zr register, and reduce the code size. But here, whether the imm is already masked is important.
If it is not masked, when the imm is like 0x8000, we miss the chance to use zr.
I don't think it is the good choice to write (imm.m_value & 0xff) / static_cast<int8_t>(imm.m_value) every time we encounter the code like the above.
If the imm is typed as TrustedImm32Masked8, we don't need to consider about the above problem.

And using TrustedImm32Masked8 as the argument type prevent us from forgetting masking the Imm eagerly.
If we forget it, it miss the chance to emit better instruction when we encounter the above case.

[Yusuke Suzuki]

Ping?

[Geoffrey Garen]

Comment on attachment 283103 [details]
Patch

Do we really need a new type to communicate that test8 only tests 8 bits?

I like clarity and readability but I think it's common in the assembler for the operation to specify or imply the bit interpretation. For example, signed compare interprets bits differently from unsigned compare, but we don't use distinct SignedImm32 vs UnsignedImm32 types in the assembler.

We would need to introduce quite a few types to meet the goal of typed arguments in the assembler: SignedImm8, UnsignedImm8,  etc.

I think in general high-level programming languages communicate bit interpretation through types but assemblers and machine code do it through instructions.

[Yusuke Suzuki]

(In reply to comment #42)
> Comment on attachment 283103 [details]
> Patch
> 
> Do we really need a new type to communicate that test8 only tests 8 bits?
> 
> I like clarity and readability but I think it's common in the assembler for
> the operation to specify or imply the bit interpretation. For example,
> signed compare interprets bits differently from unsigned compare, but we
> don't use distinct SignedImm32 vs UnsignedImm32 types in the assembler.
> 
> We would need to introduce quite a few types to meet the goal of typed
> arguments in the assembler: SignedImm8, UnsignedImm8,  etc.
> 
> I think in general high-level programming languages communicate bit
> interpretation through types but assemblers and machine code do it through
> instructions.

I think these imms need to be handled carefully. There are several examples. One is the above zr example.
```
if (!imm.m_value) {
    store8(ARM64Registers::zr, address);
    return;
}
move(imm, getCachedDataTempRegisterIDAndInvalidate());
store8(dataTempRegister, address);
```
If we misconsider the interpretation of the argument's Imm, we may accidentally emit the larger code.


In addition to the optimization case, we have the code like this,
```
Jump branch8(RelationalCondition cond, BaseIndex left, TrustedImm32 right)
{
    load8(left, ARMRegisters::S1);
    return branch32(cond, ARMRegisters::S1, right);
}
```
In the above code, masking 8bit is mandatory. Without masking, it emits incorrect code since we leverage branch32 here. `right` needs to be masked before passing it to branch32.
While x86 can handle fine grained bit interpretations through instructions (8, 16 etc.), ARM typically handles it in 32, 64 width. So we need to handle non-8bit imms so carefully to construct the macro assembler (with the same semantics) over the x86 / ARM...

If we can avoid the such cases by just inserting TrustedImm32Masked8 mechanically to the xxx8 methods' argument Imms, I thought it's reasonable choice.
That's why I introduced a new type.

But if you prefer the way taking TrustedImm32 & casting it explicitly in the methods, I'll follow that :)

[Geoffrey Garen]

> This signature gives us more context than using TrustedImm32: we can take
> 32bit value, but only 8bit is effective in this method.

If we do decide that we need a type here, I would call it TrustedImm8. All we're communicating is that we have an 8bit immediate. I don't think the provenance of the immediate is relevant -- and in fact these functions will happily accept 8bit values that have never been 32bit values.

> Whether the value is masked or not is important when selecting better
> instructions. For example, we have several code like,
> 
> ```
> if (!imm.m_value) {
>     store8(ARM64Registers::zr, address);
>     return;
> }
> move(imm, getCachedDataTempRegisterIDAndInvalidate());
> store8(dataTempRegister, address);
> ```
> 
> If the imm is 0, we can leverage the zr register, and reduce the code size.

One problem I'm having in evaluating this optimization is that I don't see any calls to  the explicit TrustedImm32Masked8 constructor in this patch. Which call sites used to miss the zr optimization, and now get it?

I'm trying to understand where this problem of 8bit values with high bits set occurs.

> I don't think it is the good choice to write (imm.m_value & 0xff) /
> static_cast<int8_t>(imm.m_value) every time we encounter the code like the
> above.
> If the imm is typed as TrustedImm32Masked8, we don't need to consider about
> the above problem.

We still have to consider this problem. You've just added a helper function to call once we've considered this problem and decided on an answer.

The helper function is convenient, which is nice, but it also tends to hide the mask operation. 

I think the question of who should apply the mask and how they should apply it depend a lot of context. Let's look at some example call sites.

[Yusuke Suzuki]

(In reply to comment #44)
> If we do decide that we need a type here, I would call it TrustedImm8. All
> we're communicating is that we have an 8bit immediate. I don't think the
> provenance of the immediate is relevant -- and in fact these functions will
> happily accept 8bit values that have never been 32bit values.
> One problem I'm having in evaluating this optimization is that I don't see
> any calls to  the explicit TrustedImm32Masked8 constructor in this patch.

Ah, sorry for my misleading explanation.
We introduced TrustedImm32Masked8, but TrustedImm32Masked8 has the implicit conversion from the TrustedImm32.
This is why I named it TrustedImm32Masked8 instead of TrustedImm8. It is TrustedImm32, but masked to 8bit.
So we can pass TrustedImm32, but through its coercion, it is masked to 8bit.
So the caller side can pass the TrustedImm32 to these methods. And the macro assembler side has the responsibility to mask it to 8bit, and it is done super eagerly (at the interface boundary) through TrustedImm32Masked8 type coercion.

So, for readability, TrustedImm32Masked8 implies to the callers that this is TrustedImm32 but only lower 8bit is effective.
For the requirement of the actual mask operations, this type coercion masks imm32 to 8bit as soon as the macro assembler uses the value.

Previously, we were confused on the meaning of this TrustedImm32 argument. The macro assembler thought that this TrustedImm32 is correctly masked to 8bit imm before it is passed. But Air thought that this macro assembler interface can accept any imm32 value, and the lower 8bit is effective. And it causes this issue.

> Which call sites used to miss the zr optimization, and now get it?
> I'm trying to understand where this problem of 8bit values with high bits
> set occurs.

Originally, this change is introduced for Air. DFG and Baseline use the form that directly call xxx8 with the 8bit imms. So this patch was not necessary!
But as Filip pointed, Air describes these 8bit imms just as 32bit values. So Air can pass a large immediate to xxx8 methods.
And if we encounter that pattern, this case works.

> > I don't think it is the good choice to write (imm.m_value & 0xff) /
> > static_cast<int8_t>(imm.m_value) every time we encounter the code like the
> > above.
> > If the imm is typed as TrustedImm32Masked8, we don't need to consider about
> > the above problem.
> 
> We still have to consider this problem. You've just added a helper function
> to call once we've considered this problem and decided on an answer.

Right. But this consideration is done at the interface of the MacroAssembler. And when we use the imms, we don't need to consider about it, correct?

> The helper function is convenient, which is nice, but it also tends to hide
> the mask operation. 

Right. But is there any problems caused due to hiding this mask operation?
Even without this type coercion, we still need to cast these imms to 8bit ASAP, right?

And if we represent the casted one as TrustedImm32 even if this is already masked, we carefully need to consider about whether this imm is already masked or not. (For example, as is often the case, if we delegate the operation to the other method with this masked TrustedImm32, in the other function, we think that the argument TrustedImm32 as masked. It is ok for the macro assembler internal function. But if it can be called from out of the macro assembler, it requires the mask operation.)

> I think the question of who should apply the mask and how they should apply
> it depend a lot of context. Let's look at some example call sites.

Right. Before this patch, the masks is done in the caller sides (out of the macro assembler).
But it is not the correct words. Actually, the callers (DFG, Baseline) always pass the 8bit imms directly, so this problem does not occur.
As the dropped assertion show, the macro assembler assumed that the given imms is already masked.

But this is not the true in Air. Now, non-8bit-imm32 can be passed.
So either the callers or the macro assembler need to have the responsibility to mask the imm32 to 8bit.

In this patch, I decide that the macro assembler side should be responsible to mask the given imm32 to 8bit. And this is done by TrustedImm32 -> TrustedImm32Masked8 type coercion. I think this is reasonable since the macro assembler accepts the imm32 value and interprets it based on its instruction (xxx8). What do you think of?

[Yusuke Suzuki]

Ping? :)

[Filip Pizlo]

I have a strong preference for not introducing a new type.

First of all, only x86 even has a notion of 8-bit immediates. I'd like us to stop adding x86-specific concepts to the MacroAssembler API. 

Second, there is no bug that we have ever seen that this new type would prevent. The only bug here is a wrong assertion. There is absolutely nothing wrong with passing an immediate that has some bits set that get ignored by the instruction. The assertion was never right. Air just made it more obvious that it was wrong. 

Finally, if we make it a policy to add a new type every time that an instrction ignores bits in an immediate then we will have an explosion of types. For example, the immediate given to a shift is really a 3-bit immediate for byte shifts, - 4-bit immediate for short shifts, a 5-bit immediate for 32-but shifts, -'d a 6-bit immediate for 64-but shifts. If we add a type for anytime some bits of a 32-bit immediate get ignored, then we should also add types for these bizarre shift immediates. But that would be wasteful and confusing. It wouldn't prevent any bugs that we have ever actually experienced. 

For these reasons I don't want us to have a new immediate type. Immediates are complicated beasts and each instruction has weird semantics for how it will interpret that immediate. Unless encoding that information in the type would actually help in some way, we shouldn't do it. In this case it doesn't help at all. We have no bugs resulting from clients of the assembler passing the wrong immediate to 8-but instructions other than the incorrect assertion, which we should simply remove.

[Yusuke Suzuki]

Hi! Thanks for your comment :)

(In reply to comment #47)
> I have a strong preference for not introducing a new type.
> 
> First of all, only x86 even has a notion of 8-bit immediates. I'd like us to
> stop adding x86-specific concepts to the MacroAssembler API. 

Right.

> Second, there is no bug that we have ever seen that this new type would
> prevent. The only bug here is a wrong assertion. There is absolutely nothing
> wrong with passing an immediate that has some bits set that get ignored by
> the instruction. The assertion was never right. Air just made it more
> obvious that it was wrong. 

No. Dropping a wrong assertion is not enough. Let's look at the real example, the current MacroAssemblerARM64::branch8.

Jump branch8(RelationalCondition cond, Address left, TrustedImm32 right)
{
    ASSERT(!(0xffffff00 & right.m_value));
    load8(left, getCachedMemoryTempRegisterIDAndInvalidate());
    return branch32(cond, memoryTempRegister, right);
}

Just dropping the assertion is not enough. This ARM64 implementation does not ignore the high bits in `right`. The casting is necessary. And at least, in my patch, that new type prevent the bug.

And interestinglly, the bugs of this type can be seen in SH4, MIPS, and ARMv7. Only x86 macro assembler is OK for just dropping the assertion. This is because only x86 has the notion of 8bit immediates in the instruction level: When emitting the instruction, the x86 assembler ignores the higher bits of immediates, while the other assemblers need to ignore it explicitly.

> Finally, if we make it a policy to add a new type every time that an
> instrction ignores bits in an immediate then we will have an explosion of
> types. For example, the immediate given to a shift is really a 3-bit
> immediate for byte shifts, - 4-bit immediate for short shifts, a 5-bit
> immediate for 32-but shifts, -'d a 6-bit immediate for 64-but shifts. If we
> add a type for anytime some bits of a 32-bit immediate get ignored, then we
> should also add types for these bizarre shift immediates. But that would be
> wasteful and confusing. It wouldn't prevent any bugs that we have ever
> actually experienced. 

Right, it is confusing. (But I think "It wouldn't prevent any bugs that we have ever actually experienced." part is wrong, see ARM64 macro assembler real example.)

> For these reasons I don't want us to have a new immediate type. Immediates
> are complicated beasts and each instruction has weird semantics for how it
> will interpret that immediate. Unless encoding that information in the type
> would actually help in some way, we shouldn't do it. In this case it doesn't
> help at all. We have no bugs resulting from clients of the assembler passing
> the wrong immediate to 8-but instructions other than the incorrect
> assertion, which we should simply remove.

Hmm, I agree to the part "introducing a new type for that is costly & confusing", but I cannot agree to the part "We have no bugs resulting from clients of the assembler passing the wrong immediate to 8-but instructions other than the incorrect assertion, which we should simply remove."

Only the x86 macro assembler is OK for just dropping the assertion. All the other macro assemblers cause the real bugs if we just drop the assertion. I don't think that this situation is "We have no bugs resulting from clients of the assembler passing the wrong immediate to 8-but instructions other than the incorrect assertion, which we should simply remove.".

I'll create the patch, dropping the assertions and inserting the necessary castings.

[Filip Pizlo]

(In reply to comment #48)
> Hi! Thanks for your comment :)
> 
> (In reply to comment #47)
> > I have a strong preference for not introducing a new type.
> > 
> > First of all, only x86 even has a notion of 8-bit immediates. I'd like us to
> > stop adding x86-specific concepts to the MacroAssembler API. 
> 
> Right.
> 
> > Second, there is no bug that we have ever seen that this new type would
> > prevent. The only bug here is a wrong assertion. There is absolutely nothing
> > wrong with passing an immediate that has some bits set that get ignored by
> > the instruction. The assertion was never right. Air just made it more
> > obvious that it was wrong. 
> 
> No. Dropping a wrong assertion is not enough. Let's look at the real
> example, the current MacroAssemblerARM64::branch8.
> 
> Jump branch8(RelationalCondition cond, Address left, TrustedImm32 right)
> {
>     ASSERT(!(0xffffff00 & right.m_value));
>     load8(left, getCachedMemoryTempRegisterIDAndInvalidate());
>     return branch32(cond, memoryTempRegister, right);
> }
> 
> Just dropping the assertion is not enough. This ARM64 implementation does
> not ignore the high bits in `right`. The casting is necessary. And at least,
> in my patch, that new type prevent the bug.
> 
> And interestinglly, the bugs of this type can be seen in SH4, MIPS, and
> ARMv7. Only x86 macro assembler is OK for just dropping the assertion. This
> is because only x86 has the notion of 8bit immediates in the instruction
> level: When emitting the instruction, the x86 assembler ignores the higher
> bits of immediates, while the other assemblers need to ignore it explicitly.

Sounds like we should fix the other assemblers then.

> 
> > Finally, if we make it a policy to add a new type every time that an
> > instrction ignores bits in an immediate then we will have an explosion of
> > types. For example, the immediate given to a shift is really a 3-bit
> > immediate for byte shifts, - 4-bit immediate for short shifts, a 5-bit
> > immediate for 32-but shifts, -'d a 6-bit immediate for 64-but shifts. If we
> > add a type for anytime some bits of a 32-bit immediate get ignored, then we
> > should also add types for these bizarre shift immediates. But that would be
> > wasteful and confusing. It wouldn't prevent any bugs that we have ever
> > actually experienced. 
> 
> Right, it is confusing. (But I think "It wouldn't prevent any bugs that we
> have ever actually experienced." part is wrong, see ARM64 macro assembler
> real example.)

If you can think to make the other assemblers use the new type, then you can think to make them do the masking.  This isn't preventing a bug, it's fixing it.  I think that a fix that doesn't introduce new types is always better than one that does, all else being equal.

> 
> > For these reasons I don't want us to have a new immediate type. Immediates
> > are complicated beasts and each instruction has weird semantics for how it
> > will interpret that immediate. Unless encoding that information in the type
> > would actually help in some way, we shouldn't do it. In this case it doesn't
> > help at all. We have no bugs resulting from clients of the assembler passing
> > the wrong immediate to 8-but instructions other than the incorrect
> > assertion, which we should simply remove.
> 
> Hmm, I agree to the part "introducing a new type for that is costly &
> confusing", but I cannot agree to the part "We have no bugs resulting from
> clients of the assembler passing the wrong immediate to 8-but instructions
> other than the incorrect assertion, which we should simply remove."

I think that fixing the bug in the other assemblers with a new type is more confusing and will lead to more maintenance hassle than fixing it by explicitly performing the masking.

Masking is a very easy-to-understand concept for C++ programmers.  You can express it natively in the language with an operation ("&").  Doing masking using a special type that performs it for you via implicit conversion is much more confusing than using the existing C++ expression for doing the same thing.

> 
> Only the x86 macro assembler is OK for just dropping the assertion. All the
> other macro assemblers cause the real bugs if we just drop the assertion. I
> don't think that this situation is "We have no bugs resulting from clients
> of the assembler passing the wrong immediate to 8-but instructions other
> than the incorrect assertion, which we should simply remove.".
> 
> I'll create the patch, dropping the assertions and inserting the necessary
> castings.

[Yusuke Suzuki]

(In reply to comment #49)]
> 
> Sounds like we should fix the other assemblers then.

OK.

> If you can think to make the other assemblers use the new type, then you can
> think to make them do the masking.  This isn't preventing a bug, it's fixing
> it. 

And, if we use this new type mechanically when we introduce a new xxx8 method (since the other xxx8 methods use this type),
it always inserts the masking and prevents this bugs, right?

> I think that a fix that doesn't introduce new types is always better
> than one that does, all else being equal.

OK. I'll insert the explicit masking.

> I think that fixing the bug in the other assemblers with a new type is more
> confusing and will lead to more maintenance hassle than fixing it by
> explicitly performing the masking.
> 
> Masking is a very easy-to-understand concept for C++ programmers.  You can
> express it natively in the language with an operation ("&").  Doing masking
> using a special type that performs it for you via implicit conversion is
> much more confusing than using the existing C++ expression for doing the
> same thing.

Yeah, the explicit masking is easy to read.
While it reduces the benefit that a new type enforces us to insert the maskings implicitly,
maybe, adding xxxx8 is not so frequent.

[Yusuke Suzuki]

Created attachment 283845 [details]
Patch

[Build Bot]

Comment on attachment 283845 [details]
Patch

Attachment 283845 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1690123

New failing tests:
webgl/1.0.2/conformance/textures/texture-mips.html

[Build Bot]

Created attachment 283846 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Yusuke Suzuki]

(In reply to comment #53)
> Created attachment 283846 [details]
> Archive of layout-test-results from ews114 for mac-yosemite
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-debug-ews.
> Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

Ah, ok. (int32_t)(int32_t & 0xff) => [0, 256). But here, we would like to generate [-128, 128). Just using `static_cast<int8_t>(...)` is better.

[Yusuke Suzuki]

Created attachment 283847 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 283847 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=283847&action=review

> Source/JavaScriptCore/assembler/MacroAssembler.h:793
> +        TrustedImm32 mask8(static_cast<int8_t>(mask.m_value));

Use `static_cast<int8_t>(...)` for casting. Not using `mask.m_value & 0xff`.
`& 0xff` is not enough. When 255 value comes, `value & 0xff` becomes 255. And the TrustedImm32 becomes 255.
But here, we would like to enforce the value to [-128, 128).

[Yusuke Suzuki]

Committed r203331: <http://trac.webkit.org/changeset/203331>