Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1916043002:
[es7] Fix "implement exponentiation operator proposal" for AIX. (Closed)
|
Created:
4 years, 8 months ago by MTBrandyberry Modified:
4 years, 7 months ago CC:
v8-reviews_googlegroups.com, JaideepBajwa Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[es7] Fix "implement exponentiation operator proposal" for AIX.
Prefer Pow() as it works around certain cases that are different in AIX's
std::pow().
TEST=mjsunit/harmony/exponentiation-operator
R=caitpotter88@gmail.com, littledan@chromium.org, adamk@chromium.org, rossberg@chromium.org
BUG=
Committed: https://crrev.com/2e4280f25af492dd5dd4bd57c621c15082b7f28c
Cr-Commit-Position: refs/heads/master@{#35775}
Patch Set 1 #Patch Set 2 : introduce Pow in utils #
Total comments: 1
Messages
Total messages: 30 (13 generated)
Description was changed from ========== [es7] Fix "implement exponentiation operator proposal" for AIX. Prefer power_double_double() as it works around certain cases that are different in AIX's std::pow(). TEST=mjsunit/harmony/exponentiation-operator R=caitpotter88@chromium.org, littledan@chromium.org, adamk@chromium.org, rossberg@chromium.org BUG= ========== to ========== [es7] Fix "implement exponentiation operator proposal" for AIX. Prefer power_double_double() as it works around certain cases that are different in AIX's std::pow(). TEST=mjsunit/harmony/exponentiation-operator R=caitpotter88@gmail.com, littledan@chromium.org, adamk@chromium.org, rossberg@chromium.org BUG= ==========
mbrandy@us.ibm.com changed reviewers: + caitpotter88@gmail.com - caitpotter88@chromium.org
PTAL
The CQ bit was checked by mbrandy@us.ibm.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1916043002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1916043002/1
On 2016/04/25 17:35:46, MTBrandyberry wrote: > PTAL If it builds, LGTM --- may need to include assembler.h or forward declare the function, though.
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 mbrandy@us.ibm.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1916043002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1916043002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/14127)
On 2016/04/25 17:40:16, caitp wrote: > On 2016/04/25 17:35:46, MTBrandyberry wrote: > > PTAL > > If it builds, LGTM --- may need to include assembler.h or forward declare the > function, though. Thanks. Build and dry run were successful. Just need owner review.
I don't think we want the Parser depending on the Assembler...maybe this
function should move to src/utils.{h,cc}?
Description was changed from ========== [es7] Fix "implement exponentiation operator proposal" for AIX. Prefer power_double_double() as it works around certain cases that are different in AIX's std::pow(). TEST=mjsunit/harmony/exponentiation-operator R=caitpotter88@gmail.com, littledan@chromium.org, adamk@chromium.org, rossberg@chromium.org BUG= ========== to ========== [es7] Fix "implement exponentiation operator proposal" for AIX. Prefer Pow() as it works around certain cases that are different in AIX's std::pow(). TEST=mjsunit/harmony/exponentiation-operator R=caitpotter88@gmail.com, littledan@chromium.org, adamk@chromium.org, rossberg@chromium.org BUG= ==========
On 2016/04/25 18:30:29, adamk wrote:
> I don't think we want the Parser depending on the Assembler...maybe this
> function should move to src/utils.{h,cc}?
Please see Patch Set 2.
The CQ bit was checked by mbrandy@us.ibm.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1916043002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1916043002/20001
https://codereview.chromium.org/1916043002/diff/20001/src/assembler.cc File src/assembler.cc (right): https://codereview.chromium.org/1916043002/diff/20001/src/assembler.cc#newcod... src/assembler.cc:1734: if (std::isnan(y) || ((x == 1 || x == -1) && std::isinf(y))) { Any reason not to move this over to Pow as well?
On 2016/04/25 19:06:57, adamk wrote: > https://codereview.chromium.org/1916043002/diff/20001/src/assembler.cc > File src/assembler.cc (right): > > https://codereview.chromium.org/1916043002/diff/20001/src/assembler.cc#newcod... > src/assembler.cc:1734: if (std::isnan(y) || ((x == 1 || x == -1) && > std::isinf(y))) { > Any reason not to move this over to Pow as well? I'll defer to the consensus here, but: a) this appeared to be specific to the assembler (per the comment) b) mjsunit/harmony/exponentiation-operator includes those inputs and std::pow appears to handle them correctly.
On 2016/04/25 19:18:34, MTBrandyberry wrote: > On 2016/04/25 19:06:57, adamk wrote: > > https://codereview.chromium.org/1916043002/diff/20001/src/assembler.cc > > File src/assembler.cc (right): > > > > > https://codereview.chromium.org/1916043002/diff/20001/src/assembler.cc#newcod... > > src/assembler.cc:1734: if (std::isnan(y) || ((x == 1 || x == -1) && > > std::isinf(y))) { > > Any reason not to move this over to Pow as well? > > I'll defer to the consensus here, but: > > a) this appeared to be specific to the assembler (per the comment) > b) mjsunit/harmony/exponentiation-operator includes those inputs and std::pow > appears to handle them correctly. If the AIX code is OK going after that specialization, then this works for me. The parser doesn't care either way, it's true, since its call will never pass NaN or Infinity (it only handles purely numeric arguments). So lgtm as is if the reordering you introduced works for your case.
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 mbrandy@us.ibm.com
The patchset sent to the CQ was uploaded after l-g-t-m from caitpotter88@gmail.com Link to the patchset: https://codereview.chromium.org/1916043002/#ps20001 (title: "introduce Pow in utils")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1916043002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1916043002/20001
Message was sent while issue was closed.
Description was changed from ========== [es7] Fix "implement exponentiation operator proposal" for AIX. Prefer Pow() as it works around certain cases that are different in AIX's std::pow(). TEST=mjsunit/harmony/exponentiation-operator R=caitpotter88@gmail.com, littledan@chromium.org, adamk@chromium.org, rossberg@chromium.org BUG= ========== to ========== [es7] Fix "implement exponentiation operator proposal" for AIX. Prefer Pow() as it works around certain cases that are different in AIX's std::pow(). TEST=mjsunit/harmony/exponentiation-operator R=caitpotter88@gmail.com, littledan@chromium.org, adamk@chromium.org, rossberg@chromium.org BUG= ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [es7] Fix "implement exponentiation operator proposal" for AIX. Prefer Pow() as it works around certain cases that are different in AIX's std::pow(). TEST=mjsunit/harmony/exponentiation-operator R=caitpotter88@gmail.com, littledan@chromium.org, adamk@chromium.org, rossberg@chromium.org BUG= ========== to ========== [es7] Fix "implement exponentiation operator proposal" for AIX. Prefer Pow() as it works around certain cases that are different in AIX's std::pow(). TEST=mjsunit/harmony/exponentiation-operator R=caitpotter88@gmail.com, littledan@chromium.org, adamk@chromium.org, rossberg@chromium.org BUG= Committed: https://crrev.com/2e4280f25af492dd5dd4bd57c621c15082b7f28c Cr-Commit-Position: refs/heads/master@{#35775} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/2e4280f25af492dd5dd4bd57c621c15082b7f28c Cr-Commit-Position: refs/heads/master@{#35775} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
