Issue 1330933002: Implements int2fp, fp2int, and fp2fp conversions for ARM32.

Issue 1330933002: Implements int2fp, fp2int, and fp2fp conversions for ARM32. (Closed)

Created:
5 years, 3 months ago by John

Modified:
5 years, 3 months ago

Reviewers:
Karl, Jim Stichnoth, ascull

CC:
native-client-reviews_googlegroups.com

Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master

Target Ref:
refs/heads/master

Visibility:
Public.

More Reviews

Description

Implements int2fp, fp2int, and fp2fp conversions for ARM32. 64-bit ints (and vectors) are future work. BUG= https://code.google.com/p/nativeclient/issues/detail?id=4076 R=kschimpf@google.com, stichnot@chromium.org Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=c31e2ed72ed52bb40e0591dd97b6d6d071631655

Patch Set 1 : Implements fp2int, int2fp, fp2fp conversions. no 64-bit integer support yet. #

Total comments: 11

Patch Set 2 : Addresses comments. #

Created: 5 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+349 lines, -22 lines)			Patch
M	src/IceConverter.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	src/IceInstARM32.h	View		2 chunks	+26 lines, -0 lines	0 comments	Download
M	src/IceInstARM32.cpp	View		2 chunks	+64 lines, -0 lines	0 comments	Download
M	src/IceTargetLoweringARM32.h	View		1 chunk	+4 lines, -0 lines	0 comments	Download
M	src/IceTargetLoweringARM32.cpp	View	1	2 chunks	+95 lines, -20 lines	0 comments	Download
M	tests_lit/llvm2ice_tests/fp.convert.ll	View	1	46 chunks	+159 lines, -1 line	0 comments	Download

Messages

Total messages: 9 (2 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Karl

Otherwise LGTM. https://codereview.chromium.org/1330933002/diff/20001/src/IceTargetLoweringARM32.h File src/IceTargetLoweringARM32.h (right): https://codereview.chromium.org/1330933002/diff/20001/src/IceTargetLoweringARM32.h#newcode393 src/IceTargetLoweringARM32.h:393: CondARM32::Cond Pred = CondARM32::AL) { Is the ...

5 years, 3 months ago (2015-09-10 16:36:10 UTC) #4

ascull

https://codereview.chromium.org/1330933002/diff/20001/src/IceInstARM32.cpp File src/IceInstARM32.cpp (right): https://codereview.chromium.org/1330933002/diff/20001/src/IceInstARM32.cpp#newcode945 src/IceInstARM32.cpp:945: } // end of anonymous namespace Why an anonymous ...

5 years, 3 months ago (2015-09-10 16:44:09 UTC) #5

Jim Stichnoth

LGTM. Can you modify the Subject and first line of Description to mention ARM? https://codereview.chromium.org/1330933002/diff/20001/src/IceTargetLoweringARM32.cpp ...

5 years, 3 months ago (2015-09-10 17:17:10 UTC) #6

John

5 years, 3 months ago (2015-09-11 12:16:48 UTC) #7

https://codereview.chromium.org/1330933002/diff/20001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1330933002/diff/20001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:945: } // end of anonymous namespace
On 2015/09/10 16:44:09, ascull wrote:
> Why an anonymous namespace function rather than a private static? How do you
> choose either way?

The choice of one versus the other is more of a personal style than a matter of
right versus wrong. In this case, this method is not part of the class'
interface -- it is an implementation detail of the InstARM32Vcvt::emit() method.
I would only declare a private static method if the method was needed anywhere
outside of the class' implementation. Private methods (static or not) are also
part of the class' interface through friendship.

https://codereview.chromium.org/1330933002/diff/20001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1330933002/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:44: stack traces. */                             
                        \
On 2015/09/10 16:44:09, ascull wrote:
> reflow

Done.

https://codereview.chromium.org/1330933002/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:2111: } else if (Dest->getType() == IceType_i64)
{
On 2015/09/10 17:17:10, stichnot wrote:
> Make this a separate "if" statement instead of "else if"?

I tend to prefer the looks of

if (Cond1) {
  Body1;
  break/return;
} else if (Cond2) {
  Body2;
  break/return;
}

versus

if (Cond1) {
  Body1;
  break/return;
}
if (Cond2) {
  Body2;
  break/return;
}

Plus, an "else" helps when first looking at the code -- particularly on IDE with
collapsible blocks -- because you know that either Body1 or Body2 will run
(never both) even before you look at the code.

It may seem I have a strong opinion on the matter, but I really don't -- I was
just justifying my choice. Done.

https://codereview.chromium.org/1330933002/diff/20001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1330933002/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.h:393: CondARM32::Cond Pred = CondARM32::AL) {
On 2015/09/10 16:36:10, Karl wrote:
> Is the optional version of Pred ever used? Would it be better to make this
> always be expclicit?

I thought about that, but then I decided to follow what the current
implementation does. It also seems appropriate to have a default predicate
argument here: in ARM asm, every predicatable instruction

foo{c} <operands>

has an implicit version

foo <operands>

that mean exactly

foo.AL <operands>

https://codereview.chromium.org/1330933002/diff/20001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/fp.convert.ll (right):

https://codereview.chromium.org/1330933002/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/fp.convert.ll:42: ; ARM32-LABEL:doubleToSigned64
On 2015/09/10 16:44:09, ascull wrote:
> Nit: space after the colon for consistency. And below.

Done.

John

Committed patchset #2 (id:40001) manually as c31e2ed72ed52bb40e0591dd97b6d6d071631655 (presubmit successful).

5 years, 3 months ago (2015-09-11 12:17:13 UTC) #8

Jim Stichnoth

5 years, 3 months ago (2015-09-11 13:49:40 UTC) #9

Message was sent while issue was closed.

https://codereview.chromium.org/1330933002/diff/20001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1330933002/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:2111: } else if (Dest->getType() == IceType_i64)
{
On 2015/09/11 12:16:48, John wrote:
> On 2015/09/10 17:17:10, stichnot wrote:
> > Make this a separate "if" statement instead of "else if"?
> 
> I tend to prefer the looks of
> 
> if (Cond1) {
>   Body1;
>   break/return;
> } else if (Cond2) {
>   Body2;
>   break/return;
> }
> 
> versus
> 
> if (Cond1) {
>   Body1;
>   break/return;
> }
> if (Cond2) {
>   Body2;
>   break/return;
> }
> 
> Plus, an "else" helps when first looking at the code -- particularly on IDE
with
> collapsible blocks -- because you know that either Body1 or Body2 will run
> (never both) even before you look at the code.
> 
> It may seem I have a strong opinion on the matter, but I really don't -- I was
> just justifying my choice. Done.

I brought it up because LLVM seems to opine pretty strongly on the matter.
http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages