Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2033413002:
[crankshaft] Fix DoDeferredMathAbsTaggedHeapNumber overwriting the context with some temporary valu… (Closed)
Description[crankshaft] Fix DoDeferredMathAbsTaggedHeapNumber overwriting the context with some temporary value.
BUG=v8:5067
Committed: https://crrev.com/36807f064a44e15fbfd1b4e5907d96545f23beea
Cr-Commit-Position: refs/heads/master@{#36738}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Update #
Total comments: 6
Patch Set 3 : Update. #Patch Set 4 : Update. #
Messages
Total messages: 21 (7 generated)
epertoso@chromium.org changed reviewers: + jarin@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/2033413002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2033413002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2033413002/diff/1/src/crankshaft/ia32/lithium... File src/crankshaft/ia32/lithium-codegen-ia32.cc (right): https://codereview.chromium.org/2033413002/diff/1/src/crankshaft/ia32/lithium... src/crankshaft/ia32/lithium-codegen-ia32.cc:3087: } It is not obvious that tmp/tmp2 cannot alias the input_reg after this exercise. Could we DCHECK that here (and the other ports) for documentation?
https://codereview.chromium.org/2033413002/diff/1/src/crankshaft/ia32/lithium... File src/crankshaft/ia32/lithium-codegen-ia32.cc (right): https://codereview.chromium.org/2033413002/diff/1/src/crankshaft/ia32/lithium... src/crankshaft/ia32/lithium-codegen-ia32.cc:3087: } On 2016/06/03 at 13:54:56, Jarin wrote: > It is not obvious that tmp/tmp2 cannot alias the input_reg after this exercise. > > Could we DCHECK that here (and the other ports) for documentation? Changed it a bit. Feel free to Commit if you still like it.
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/2033413002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2033413002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2033413002/diff/20001/src/crankshaft/x87/lith... File src/crankshaft/x87/lithium-codegen-x87.cc (right): https://codereview.chromium.org/2033413002/diff/20001/src/crankshaft/x87/lith... src/crankshaft/x87/lithium-codegen-x87.cc:3364: Register::from_code(base::bits::CountTrailingZeros32(available_regs)); I am not quite happy about hardcoding the integer values of the register codes here. Let me think about alternatives...
How about the changes below? https://codereview.chromium.org/2033413002/diff/20001/src/crankshaft/ia32/lit... File src/crankshaft/ia32/lithium-codegen-ia32.cc (right): https://codereview.chromium.org/2033413002/diff/20001/src/crankshaft/ia32/lit... src/crankshaft/ia32/lithium-codegen-ia32.cc:3075: uint32_t available_regs = 0xf; uint32_t available_regs = eax.bit() | ebx.bit() | ecx.bit() | edx.bit(); https://codereview.chromium.org/2033413002/diff/20001/src/crankshaft/ia32/lit... src/crankshaft/ia32/lithium-codegen-ia32.cc:3076: if (input_reg.code() < 4) available_regs ^= input_reg.bit(); available_regs &= ~input_reg.bit(); ? https://codereview.chromium.org/2033413002/diff/20001/src/crankshaft/ia32/lit... src/crankshaft/ia32/lithium-codegen-ia32.cc:3081: if (context_reg.code() < 4) available_regs ^= context_reg.bit(); available_regs &= ~context_reg.bit(); https://codereview.chromium.org/2033413002/diff/20001/src/crankshaft/ia32/lit... src/crankshaft/ia32/lithium-codegen-ia32.cc:3086: available_regs ^= tmp.bit(); For consistency, may even here: DCHECK(available_regs & tmp.bit()); available_regs &= ~tmp.bit(); https://codereview.chromium.org/2033413002/diff/20001/src/crankshaft/ia32/lit... src/crankshaft/ia32/lithium-codegen-ia32.cc:3088: Register::from_code(base::bits::CountTrailingZeros32(available_regs)); DCHECK(available_regs & tmp2.bit());
On 2016/06/06 at 08:09:22, jarin wrote: > How about the changes below? > > https://codereview.chromium.org/2033413002/diff/20001/src/crankshaft/ia32/lit... > File src/crankshaft/ia32/lithium-codegen-ia32.cc (right): > > https://codereview.chromium.org/2033413002/diff/20001/src/crankshaft/ia32/lit... > src/crankshaft/ia32/lithium-codegen-ia32.cc:3075: uint32_t available_regs = 0xf; > uint32_t available_regs = eax.bit() | ebx.bit() | ecx.bit() | edx.bit(); > > https://codereview.chromium.org/2033413002/diff/20001/src/crankshaft/ia32/lit... > src/crankshaft/ia32/lithium-codegen-ia32.cc:3076: if (input_reg.code() < 4) available_regs ^= input_reg.bit(); > available_regs &= ~input_reg.bit(); ? > > https://codereview.chromium.org/2033413002/diff/20001/src/crankshaft/ia32/lit... > src/crankshaft/ia32/lithium-codegen-ia32.cc:3081: if (context_reg.code() < 4) available_regs ^= context_reg.bit(); > available_regs &= ~context_reg.bit(); > > https://codereview.chromium.org/2033413002/diff/20001/src/crankshaft/ia32/lit... > src/crankshaft/ia32/lithium-codegen-ia32.cc:3086: available_regs ^= tmp.bit(); > For consistency, may even here: > > DCHECK(available_regs & tmp.bit()); > available_regs &= ~tmp.bit(); > > https://codereview.chromium.org/2033413002/diff/20001/src/crankshaft/ia32/lit... > src/crankshaft/ia32/lithium-codegen-ia32.cc:3088: Register::from_code(base::bits::CountTrailingZeros32(available_regs)); > DCHECK(available_regs & tmp2.bit()); Done (except the DCHECK).
On 2016/06/06 09:06:14, epertoso wrote: > On 2016/06/06 at 08:09:22, jarin wrote: > > How about the changes below? > > > > > https://codereview.chromium.org/2033413002/diff/20001/src/crankshaft/ia32/lit... > > File src/crankshaft/ia32/lithium-codegen-ia32.cc (right): > > > > > https://codereview.chromium.org/2033413002/diff/20001/src/crankshaft/ia32/lit... > > src/crankshaft/ia32/lithium-codegen-ia32.cc:3075: uint32_t available_regs = > 0xf; > > uint32_t available_regs = eax.bit() | ebx.bit() | ecx.bit() | edx.bit(); > > > > > https://codereview.chromium.org/2033413002/diff/20001/src/crankshaft/ia32/lit... > > src/crankshaft/ia32/lithium-codegen-ia32.cc:3076: if (input_reg.code() < 4) > available_regs ^= input_reg.bit(); > > available_regs &= ~input_reg.bit(); ? > > > > > https://codereview.chromium.org/2033413002/diff/20001/src/crankshaft/ia32/lit... > > src/crankshaft/ia32/lithium-codegen-ia32.cc:3081: if (context_reg.code() < 4) > available_regs ^= context_reg.bit(); > > available_regs &= ~context_reg.bit(); > > > > > https://codereview.chromium.org/2033413002/diff/20001/src/crankshaft/ia32/lit... > > src/crankshaft/ia32/lithium-codegen-ia32.cc:3086: available_regs ^= tmp.bit(); > > For consistency, may even here: > > > > DCHECK(available_regs & tmp.bit()); > > available_regs &= ~tmp.bit(); > > > > > https://codereview.chromium.org/2033413002/diff/20001/src/crankshaft/ia32/lit... > > src/crankshaft/ia32/lithium-codegen-ia32.cc:3088: > Register::from_code(base::bits::CountTrailingZeros32(available_regs)); > > DCHECK(available_regs & tmp2.bit()); > > Done (except the DCHECK). Awesome. Still LGTM.
The CQ bit was checked by epertoso@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2033413002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [crankshaft] Fix DoDeferredMathAbsTaggedHeapNumber overwriting the context with some temporary value. BUG=v8:5067 ========== to ========== [crankshaft] Fix DoDeferredMathAbsTaggedHeapNumber overwriting the context with some temporary value. BUG=v8:5067 Committed: https://crrev.com/36807f064a44e15fbfd1b4e5907d96545f23beea Cr-Commit-Position: refs/heads/master@{#36738} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/36807f064a44e15fbfd1b4e5907d96545f23beea Cr-Commit-Position: refs/heads/master@{#36738} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
