Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2666283002:
[turbofan] Constant propagation for JumpIfFalse/JumpIfTrue. (Closed)
|
Created:
3 years, 10 months ago by Benedikt Meurer Modified:
3 years, 10 months ago Reviewers:
Jarin CC:
v8-reviews_googlegroups.com, rmcilroy Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[turbofan] Constant propagation for JumpIfFalse/JumpIfTrue.
The JumpIfFalse and JumpIfTrue bytecodes test the accumulator, and
branch based on whether the accumulator is true or false (no other
value allowed, and in fact TurboFan would blow up if you would pass
anything else, since Branch operator can only deal with Boolean).
So for either branch we know exactly the value of the accumulator,
and we can update the environment to this constant value instead.
This helps to avoid the useless bit materialization that currently
happens when || or && is being used in a value context.
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_win64_dbg
R=jarin@chromium.org
BUG=v8:5267
Review-Url: https://codereview.chromium.org/2666283002
Cr-Original-Commit-Position: refs/heads/master@{#42843}
Committed: https://chromium.googlesource.com/v8/v8/+/158ac9287193f315342ad31c38fe451620d176eb
Review-Url: https://codereview.chromium.org/2666283002
Cr-Commit-Position: refs/heads/master@{#42934}
Committed: https://chromium.googlesource.com/v8/v8/+/51ed12f96e5700b7ca5a89729574c719835a46d7
Patch Set 1 #Patch Set 2 : Add CHECK to verifier. #Patch Set 3 : Also add a DCHECK to SimplifiedLowering. #Patch Set 4 : Also add proper guards to Ignition. #Patch Set 5 : REBASE #
Messages
Total messages: 38 (27 generated)
The CQ bit was checked by bmeurer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
You need to do some serious documentation on the invariants that you are cementing here. Also, I am not quite sure why you say that Branch can only handle Booleans. As far as I can see, representation selection and the verifier do not make assumptions on the condition input of a branch. In fact, representation selection would work with other inputs, too. If you believe the input is always boolean, please add DCHECKs to that effect at least to the representation selection.
On 2017/02/01 08:10:11, Jarin wrote: > You need to do some serious documentation on the invariants that you are > cementing here. > > Also, I am not quite sure why you say that Branch can only handle Booleans. As > far as I can see, representation selection and the verifier do not make > assumptions on the condition input of a branch. In fact, representation > selection would work with other inputs, too. > > If you believe the input is always boolean, please add DCHECKs to that effect at > least to the representation selection. The representation selection works on inputs that are not necessarily booleans because we propagate the truncation. But still a Branch with a non-Boolean input is not supported, and will never be supported again. I'll add a CHECK to the verifier to assert this.
The CQ bit was checked by bmeurer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bmeurer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bmeurer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Lovely! lgtm.
The CQ bit was unchecked by bmeurer@chromium.org
The CQ bit was checked by bmeurer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485942188111710,
"parent_rev": "c67dc7e2431314542a6c24f948bdc043027365d3", "commit_rev":
"158ac9287193f315342ad31c38fe451620d176eb"}
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Constant propagation for JumpIfFalse/JumpIfTrue. The JumpIfFalse and JumpIfTrue bytecodes test the accumulator, and branch based on whether the accumulator is true or false (no other value allowed, and in fact TurboFan would blow up if you would pass anything else, since Branch operator can only deal with Boolean). So for either branch we know exactly the value of the accumulator, and we can update the environment to this constant value instead. This helps to avoid the useless bit materialization that currently happens when || or && is being used in a value context. R=jarin@chromium.org BUG=v8:5267 ========== to ========== [turbofan] Constant propagation for JumpIfFalse/JumpIfTrue. The JumpIfFalse and JumpIfTrue bytecodes test the accumulator, and branch based on whether the accumulator is true or false (no other value allowed, and in fact TurboFan would blow up if you would pass anything else, since Branch operator can only deal with Boolean). So for either branch we know exactly the value of the accumulator, and we can update the environment to this constant value instead. This helps to avoid the useless bit materialization that currently happens when || or && is being used in a value context. R=jarin@chromium.org BUG=v8:5267 Review-Url: https://codereview.chromium.org/2666283002 Cr-Commit-Position: refs/heads/master@{#42843} Committed: https://chromium.googlesource.com/v8/v8/+/158ac9287193f315342ad31c38fe451620d... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/v8/v8/+/158ac9287193f315342ad31c38fe451620d...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2668933002/ by bmeurer@chromium.org. The reason for reverting is: Breaks win64 it seems..
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Constant propagation for JumpIfFalse/JumpIfTrue. The JumpIfFalse and JumpIfTrue bytecodes test the accumulator, and branch based on whether the accumulator is true or false (no other value allowed, and in fact TurboFan would blow up if you would pass anything else, since Branch operator can only deal with Boolean). So for either branch we know exactly the value of the accumulator, and we can update the environment to this constant value instead. This helps to avoid the useless bit materialization that currently happens when || or && is being used in a value context. R=jarin@chromium.org BUG=v8:5267 Review-Url: https://codereview.chromium.org/2666283002 Cr-Commit-Position: refs/heads/master@{#42843} Committed: https://chromium.googlesource.com/v8/v8/+/158ac9287193f315342ad31c38fe451620d... ========== to ========== [turbofan] Constant propagation for JumpIfFalse/JumpIfTrue. The JumpIfFalse and JumpIfTrue bytecodes test the accumulator, and branch based on whether the accumulator is true or false (no other value allowed, and in fact TurboFan would blow up if you would pass anything else, since Branch operator can only deal with Boolean). So for either branch we know exactly the value of the accumulator, and we can update the environment to this constant value instead. This helps to avoid the useless bit materialization that currently happens when || or && is being used in a value context. R=jarin@chromium.org BUG=v8:5267 Review-Url: https://codereview.chromium.org/2666283002 Cr-Commit-Position: refs/heads/master@{#42843} Committed: https://chromium.googlesource.com/v8/v8/+/158ac9287193f315342ad31c38fe451620d... ==========
The CQ bit was checked by bmeurer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [turbofan] Constant propagation for JumpIfFalse/JumpIfTrue. The JumpIfFalse and JumpIfTrue bytecodes test the accumulator, and branch based on whether the accumulator is true or false (no other value allowed, and in fact TurboFan would blow up if you would pass anything else, since Branch operator can only deal with Boolean). So for either branch we know exactly the value of the accumulator, and we can update the environment to this constant value instead. This helps to avoid the useless bit materialization that currently happens when || or && is being used in a value context. R=jarin@chromium.org BUG=v8:5267 Review-Url: https://codereview.chromium.org/2666283002 Cr-Commit-Position: refs/heads/master@{#42843} Committed: https://chromium.googlesource.com/v8/v8/+/158ac9287193f315342ad31c38fe451620d... ========== to ========== [turbofan] Constant propagation for JumpIfFalse/JumpIfTrue. The JumpIfFalse and JumpIfTrue bytecodes test the accumulator, and branch based on whether the accumulator is true or false (no other value allowed, and in fact TurboFan would blow up if you would pass anything else, since Branch operator can only deal with Boolean). So for either branch we know exactly the value of the accumulator, and we can update the environment to this constant value instead. This helps to avoid the useless bit materialization that currently happens when || or && is being used in a value context. CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_win64_dbg R=jarin@chromium.org BUG=v8:5267 Review-Url: https://codereview.chromium.org/2666283002 Cr-Commit-Position: refs/heads/master@{#42843} Committed: https://chromium.googlesource.com/v8/v8/+/158ac9287193f315342ad31c38fe451620d... ==========
The CQ bit was unchecked by bmeurer@chromium.org
The CQ bit was checked by bmeurer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org Link to the patchset: https://codereview.chromium.org/2666283002/#ps80001 (title: "REBASE")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_win64_dbg on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_dbg/builds/548)
The CQ bit was checked by bmeurer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486156716636150,
"parent_rev": "4675c975677661d73645cafe3e79229c332eb0c8", "commit_rev":
"51ed12f96e5700b7ca5a89729574c719835a46d7"}
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Constant propagation for JumpIfFalse/JumpIfTrue. The JumpIfFalse and JumpIfTrue bytecodes test the accumulator, and branch based on whether the accumulator is true or false (no other value allowed, and in fact TurboFan would blow up if you would pass anything else, since Branch operator can only deal with Boolean). So for either branch we know exactly the value of the accumulator, and we can update the environment to this constant value instead. This helps to avoid the useless bit materialization that currently happens when || or && is being used in a value context. CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_win64_dbg R=jarin@chromium.org BUG=v8:5267 Review-Url: https://codereview.chromium.org/2666283002 Cr-Commit-Position: refs/heads/master@{#42843} Committed: https://chromium.googlesource.com/v8/v8/+/158ac9287193f315342ad31c38fe451620d... ========== to ========== [turbofan] Constant propagation for JumpIfFalse/JumpIfTrue. The JumpIfFalse and JumpIfTrue bytecodes test the accumulator, and branch based on whether the accumulator is true or false (no other value allowed, and in fact TurboFan would blow up if you would pass anything else, since Branch operator can only deal with Boolean). So for either branch we know exactly the value of the accumulator, and we can update the environment to this constant value instead. This helps to avoid the useless bit materialization that currently happens when || or && is being used in a value context. CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_win64_dbg R=jarin@chromium.org BUG=v8:5267 Review-Url: https://codereview.chromium.org/2666283002 Cr-Original-Commit-Position: refs/heads/master@{#42843} Committed: https://chromium.googlesource.com/v8/v8/+/158ac9287193f315342ad31c38fe451620d... Review-Url: https://codereview.chromium.org/2666283002 Cr-Commit-Position: refs/heads/master@{#42934} Committed: https://chromium.googlesource.com/v8/v8/+/51ed12f96e5700b7ca5a89729574c719835... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/v8/v8/+/51ed12f96e5700b7ca5a89729574c719835... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
