Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1948453002:
[turbofan] Take the immediate size in account when narrowing ia32/x64 word comparison operators. (Closed)
Description[turbofan] Take the immediate size in account when narrowing ia32/x64 word comparison operators.
Before this patch, we would emit a cmp or test with a memory operand only if both of the operands in the IR were loads. Now if either of them is a load and the other one is an immediate, we can use a memory operand if the load representation machine size is wide enough to represent the latter.
Committed: https://crrev.com/a0543313dbd46b0c2e72c91ee3488a7dc6db73e4
Cr-Commit-Position: refs/heads/master@{#36009}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Update. #
Messages
Total messages: 23 (9 generated)
epertoso@chromium.org changed reviewers: + bmeurer@chromium.org
The CQ bit was checked by epertoso@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1948453002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1948453002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [turbofan] Take the immediate size in account when narrowing ia32/x64 word coparison operators. Before this patch, we would emit a cmp or test with a memory operand only if both of the operands in the IR were loads. Now if either of them is a load and the other one is an immediate, we can use a memory operand if the load representation machine size is wide enough to represent the latter. ========== to ========== [turbofan] Take the immediate size in account when narrowing ia32/x64 word comparison operators. Before this patch, we would emit a cmp or test with a memory operand only if both of the operands in the IR were loads. Now if either of them is a load and the other one is an immediate, we can use a memory operand if the load representation machine size is wide enough to represent the latter. ==========
LGTM with comments. https://codereview.chromium.org/1948453002/diff/1/src/compiler/ia32/instructi... File src/compiler/ia32/instruction-selector-ia32.cc (right): https://codereview.chromium.org/1948453002/diff/1/src/compiler/ia32/instructi... src/compiler/ia32/instruction-selector-ia32.cc:1175: bool GetMachineRepresentation(Node* node, Maybe rename this to something like InferMachineRepresentation? https://codereview.chromium.org/1948453002/diff/1/src/compiler/ia32/instructi... src/compiler/ia32/instruction-selector-ia32.cc:1184: uint32_t value = OpParameter<uint32_t>(node->op()); UBSAN will not be happy with this OpParameter. You need to use OpParameter<int32_t> https://codereview.chromium.org/1948453002/diff/1/src/compiler/x64/instructio... File src/compiler/x64/instruction-selector-x64.cc (right): https://codereview.chromium.org/1948453002/diff/1/src/compiler/x64/instructio... src/compiler/x64/instruction-selector-x64.cc:1503: bool GetMachineRepresentation(Node* node, See ia32 comment. https://codereview.chromium.org/1948453002/diff/1/src/compiler/x64/instructio... src/compiler/x64/instruction-selector-x64.cc:1512: value = OpParameter<uint32_t>(node->op()); See ia32 comment. https://codereview.chromium.org/1948453002/diff/1/src/compiler/x64/instructio... src/compiler/x64/instruction-selector-x64.cc:1515: value = OpParameter<uint64_t>(node->op()); See ia32 comment.
https://codereview.chromium.org/1948453002/diff/1/src/compiler/ia32/instructi... File src/compiler/ia32/instruction-selector-ia32.cc (right): https://codereview.chromium.org/1948453002/diff/1/src/compiler/ia32/instructi... src/compiler/ia32/instruction-selector-ia32.cc:1175: bool GetMachineRepresentation(Node* node, On 2016/05/04 at 04:05:47, Benedikt Meurer wrote: > Maybe rename this to something like InferMachineRepresentation? Done. https://codereview.chromium.org/1948453002/diff/1/src/compiler/ia32/instructi... src/compiler/ia32/instruction-selector-ia32.cc:1184: uint32_t value = OpParameter<uint32_t>(node->op()); On 2016/05/04 at 04:05:47, Benedikt Meurer wrote: > UBSAN will not be happy with this OpParameter. You need to use OpParameter<int32_t> Done. Didn't see it was a reinterpret_cast. https://codereview.chromium.org/1948453002/diff/1/src/compiler/x64/instructio... File src/compiler/x64/instruction-selector-x64.cc (right): https://codereview.chromium.org/1948453002/diff/1/src/compiler/x64/instructio... src/compiler/x64/instruction-selector-x64.cc:1503: bool GetMachineRepresentation(Node* node, On 2016/05/04 at 04:05:47, Benedikt Meurer wrote: > See ia32 comment. Done. https://codereview.chromium.org/1948453002/diff/1/src/compiler/x64/instructio... src/compiler/x64/instruction-selector-x64.cc:1512: value = OpParameter<uint32_t>(node->op()); On 2016/05/04 at 04:05:47, Benedikt Meurer wrote: > See ia32 comment. Done. https://codereview.chromium.org/1948453002/diff/1/src/compiler/x64/instructio... src/compiler/x64/instruction-selector-x64.cc:1515: value = OpParameter<uint64_t>(node->op()); On 2016/05/04 at 04:05:47, Benedikt Meurer wrote: > See ia32 comment. Done.
The CQ bit was checked by epertoso@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org Link to the patchset: https://codereview.chromium.org/1948453002/#ps20001 (title: "Update.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1948453002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1948453002/20001
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Take the immediate size in account when narrowing ia32/x64 word comparison operators. Before this patch, we would emit a cmp or test with a memory operand only if both of the operands in the IR were loads. Now if either of them is a load and the other one is an immediate, we can use a memory operand if the load representation machine size is wide enough to represent the latter. ========== to ========== [turbofan] Take the immediate size in account when narrowing ia32/x64 word comparison operators. Before this patch, we would emit a cmp or test with a memory operand only if both of the operands in the IR were loads. Now if either of them is a load and the other one is an immediate, we can use a memory operand if the load representation machine size is wide enough to represent the latter. ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Take the immediate size in account when narrowing ia32/x64 word comparison operators. Before this patch, we would emit a cmp or test with a memory operand only if both of the operands in the IR were loads. Now if either of them is a load and the other one is an immediate, we can use a memory operand if the load representation machine size is wide enough to represent the latter. ========== to ========== [turbofan] Take the immediate size in account when narrowing ia32/x64 word comparison operators. Before this patch, we would emit a cmp or test with a memory operand only if both of the operands in the IR were loads. Now if either of them is a load and the other one is an immediate, we can use a memory operand if the load representation machine size is wide enough to represent the latter. Committed: https://crrev.com/a0543313dbd46b0c2e72c91ee3488a7dc6db73e4 Cr-Commit-Position: refs/heads/master@{#36009} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/a0543313dbd46b0c2e72c91ee3488a7dc6db73e4 Cr-Commit-Position: refs/heads/master@{#36009}
Message was sent while issue was closed.
jfb@chromium.org changed reviewers: + jfb@chromium.org
Message was sent while issue was closed.
I believe this change causes failures on the WebAssembly waterfall wasm-stat.us. I bisected it to: FAIL a054331 [turbofan] Take the immediate size in account when narrowing ia32/x64 word comparison operators. OK f82b337 [wasm] Fix for 608630: allow proxies as FFI. To repro the 3 failing tests: wget https://storage.googleapis.com/wasm-llvm/builds/git/wasm-torture-/b/build/sla... tar -xvf emscripten_config_vanilla-6097.tbz2 (cd ./emwasm-torture-out && /path/to/d8 --expose-wasm 20010129-1.c.js) ; echo $? (cd ./emwasm-torture-out && /path/to/d8 --expose-wasm 990326-1.c.js) ; echo $? (cd ./emwasm-torture-out && /path/to/d8 --expose-wasm pr46309.c.js) ; echo $? This is a sample failing log: https://wasm-stat.us/builders/linux/builds/6130/steps/Execute%20LLVM%20Tortur... It's not very useful because d8 simply segfaults. Notice that the wget command above is for build 6097 which was the last green build (d8 didn't have the bug back then). Some intervening failures on the integration waterfall were caused by LLVM test failures, which is why it took a while for the d8 failure to be apparent. Could this issue be fixed, or this patch reverted? It would also be good to figure out a better way to test wasm builds, maybe we should have a snapshot of the wasm torture tests (the waterfall has a bunch).
Message was sent while issue was closed.
On 2016/05/05 00:28:22, JF wrote: > I believe this change causes failures on the WebAssembly waterfall wasm-stat.us. > > I bisected it to: > FAIL a054331 [turbofan] Take the immediate size in account when narrowing > ia32/x64 word comparison operators. > OK f82b337 [wasm] Fix for 608630: allow proxies as FFI. > > To repro the 3 failing tests: > wget > https://storage.googleapis.com/wasm-llvm/builds/git/wasm-torture-/b/build/sla... > tar -xvf emscripten_config_vanilla-6097.tbz2 > (cd ./emwasm-torture-out && /path/to/d8 --expose-wasm 20010129-1.c.js) ; echo > $? > (cd ./emwasm-torture-out && /path/to/d8 --expose-wasm 990326-1.c.js) ; echo $? > (cd ./emwasm-torture-out && /path/to/d8 --expose-wasm pr46309.c.js) ; echo $? > > This is a sample failing log: > https://wasm-stat.us/builders/linux/builds/6130/steps/Execute%20LLVM%20Tortur... > It's not very useful because d8 simply segfaults. > > Notice that the wget command above is for build 6097 which was the last green > build (d8 didn't have the bug back then). Some intervening failures on the > integration waterfall were caused by LLVM test failures, which is why it took a > while for the d8 failure to be apparent. > > Could this issue be fixed, or this patch reverted? It would also be good to > figure out a better way to test wasm builds, maybe we should have a snapshot of > the wasm torture tests (the waterfall has a bunch). I marked the 3 tests as known broken in the waterfall for now, but something is still broken so it still needs to get fixed.
Message was sent while issue was closed.
On 2016/05/05 18:25:47, JF wrote: > On 2016/05/05 00:28:22, JF wrote: > > I believe this change causes failures on the WebAssembly waterfall > wasm-stat.us. > > > > I bisected it to: > > FAIL a054331 [turbofan] Take the immediate size in account when narrowing > > ia32/x64 word comparison operators. > > OK f82b337 [wasm] Fix for 608630: allow proxies as FFI. > > > > To repro the 3 failing tests: > > wget > > > https://storage.googleapis.com/wasm-llvm/builds/git/wasm-torture-/b/build/sla... > > tar -xvf emscripten_config_vanilla-6097.tbz2 > > (cd ./emwasm-torture-out && /path/to/d8 --expose-wasm 20010129-1.c.js) ; > echo > > $? > > (cd ./emwasm-torture-out && /path/to/d8 --expose-wasm 990326-1.c.js) ; echo > $? > > (cd ./emwasm-torture-out && /path/to/d8 --expose-wasm pr46309.c.js) ; echo > $? > > > > This is a sample failing log: > > > https://wasm-stat.us/builders/linux/builds/6130/steps/Execute%20LLVM%20Tortur... > > It's not very useful because d8 simply segfaults. > > > > Notice that the wget command above is for build 6097 which was the last green > > build (d8 didn't have the bug back then). Some intervening failures on the > > integration waterfall were caused by LLVM test failures, which is why it took > a > > while for the d8 failure to be apparent. > > > > Could this issue be fixed, or this patch reverted? It would also be good to > > figure out a better way to test wasm builds, maybe we should have a snapshot > of > > the wasm torture tests (the waterfall has a bunch). > > I marked the 3 tests as known broken in the waterfall for now, but something is > still broken so it still needs to get fixed. Yeah, it looks like the WebAssembly AngryBots demo is crashing now too. I bisected it to the Chromium v8 roll that contains this change.
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/1959603002/ by titzer@chromium.org. The reason for reverting is: Breaks WASM; please also add tests when relanding..
Message was sent while issue was closed.
On 2016/05/06 07:49:50, titzer wrote: > A revert of this CL (patchset #2 id:20001) has been created in > https://codereview.chromium.org/1959603002/ by mailto:titzer@chromium.org. > > The reason for reverting is: Breaks WASM; please also add tests when relanding.. Thanks! This fixed the waterfall. I haven't checked whether it fixed AngryBots.
Message was sent while issue was closed.
On 2016/05/06 at 16:24:42, jfb wrote: > On 2016/05/06 07:49:50, titzer wrote: > > A revert of this CL (patchset #2 id:20001) has been created in > > https://codereview.chromium.org/1959603002/ by mailto:titzer@chromium.org. > > > > The reason for reverting is: Breaks WASM; please also add tests when relanding.. > > Thanks! This fixed the waterfall. I haven't checked whether it fixed AngryBots. Turns out earlier this year I introduced a bug in assembler-x64.cc. I've added a test and fixed the assembler in http://crrev.com/1962563003. The 3 tests mentioned above are now passing again. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
