Subzero. ARM32. Vector lowering. Subtract.

Subzero. ARM32. Vector lowering. Subtract.

This CL also changes UnimplementedLoweringError to display the name of
the unimplemented instruction.

Improve test coverage for ARM32 vector load instructions.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4076
R=jpp@chromium.org, kschimpf@google.com

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=e5727b836091b786dd069f582aa48452620e600e

[Eric Holk, 2016-01-26 18:54:03]

eholk@chromium.org changed reviewers:
+ jpp@chromium.org, kschimpf@google.com, sehr@chromium.org,
stichnot@chromium.org

[John, 2016-01-26 19:07:17]

https://codereview.chromium.org/1639923002/diff/1/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/1639923002/diff/1/src/IceInst.h#newcode79
src/IceInst.h:79: IceString getInstName() const {
Can you move the method to IceInst.cpp?

https://codereview.chromium.org/1639923002/diff/1/src/IceInst.h#newcode79
src/IceInst.h:79: IceString getInstName() const {
this is really useful, but does not belong in this patch. can you move it to its
own cl?

This is also going to increase what's called minimal mode (make MINIMAL=1),
which builds the binary shipped to clients. I would suggest you to add a guard

  if (!BuildDefs::dump())
    return "";

I am not sure getInstName() conveys the purpose of this method. It returns the
instruction **type**, but its name, converted to a string, but I can't think of
anything better.

https://codereview.chromium.org/1639923002/diff/1/src/IceInst.h#newcode81
src/IceInst.h:81: case Unreachable:
I would suggest the following:

switch (Kind) {
#define CASE(InstKind) case InstKind: return #InstKind;
  CASE(Unreachable)
  CASE(Alloca)
  CASE(Arithmetic)
  ...
  CASE(JumpTable)
  default:
  CASE(Target)
#undef X
}

the default is very important

https://codereview.chromium.org/1639923002/diff/1/src/IceInst.h#newcode346
src/IceInst.h:346: IceString getInstName() const {
add the

if (!BuildDefs::dump()) return "";

and move this to the .cpp file.

https://codereview.chromium.org/1639923002/diff/1/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1639923002/diff/1/src/IceInstARM32.cpp#newcod...
src/IceInstARM32.cpp:651: Asm->setNeedsTextFixup();
remove Asm->setNeedsTextFixup(); and invoke emitUsingTextFixup() here.

https://codereview.chromium.org/1639923002/diff/1/src/IceInstARM32.cpp#newcod...
src/IceInstARM32.cpp:655: Asm->vsubs(getDest(), getSrc(0), getSrc(1),
CondARM32::AL);
move the assert below Asm->vsub()

https://codereview.chromium.org/1639923002/diff/1/src/IceInstARM32.cpp#newcod...
src/IceInstARM32.cpp:662: if (Asm->needsTextFixup()) {
remove this

https://codereview.chromium.org/1639923002/diff/1/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/1639923002/diff/1/src/IceTargetLowering.h#new...
src/IceTargetLowering.h:63: llvm_unreachable(StrBuf.str().c_str());             
                    \
just do

llvm_unreachable("Not yet implemented: " + Instr->getInstName());

[Jim Stichnoth, 2016-01-26 19:13:10]

https://codereview.chromium.org/1639923002/diff/1/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/1639923002/diff/1/src/IceInst.h#newcode79
src/IceInst.h:79: IceString getInstName() const {
Could you also change the dump() methods in IceInst.cpp to use this method
instead of hard-coded strings?  This would also mean lower-casing the returned
strings in this method.

https://codereview.chromium.org/1639923002/diff/1/src/IceInst.h#newcode79
src/IceInst.h:79: IceString getInstName() const {
Add the following to the beginning of this method:

if (!BuildDefs::dump())
  return "???";

or something to that effect.  This reduces the translator size in a MINIMAL
build.

https://codereview.chromium.org/1639923002/diff/1/src/IceInst.h#newcode130
src/IceInst.h:130: return "Target";
Add an assert:
  assert(Kind >= Target);
just to make sure that newly added high-level instruction kinds don't forget to
add their string here.

https://codereview.chromium.org/1639923002/diff/1/src/IceInst.h#newcode346
src/IceInst.h:346: IceString getInstName() const {
Are you shadowing a nonvirtual method name in a base class?  That seems a little
dangerous or error-prone.  Should this be virtual, with override or final here?

[Eric Holk, 2016-01-26 22:49:24]

I split the change about putting the instruction name in the error message into
a separate CL and applied this feedback there.

The new CL is https://codereview.chromium.org/1639063002

I'll update this CL shortly with the vector subtraction changes.

https://codereview.chromium.org/1639923002/diff/1/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/1639923002/diff/1/src/IceInst.h#newcode79
src/IceInst.h:79: IceString getInstName() const {
On 2016/01/26 19:07:17, John wrote:
> Can you move the method to IceInst.cpp?

Sure.

https://codereview.chromium.org/1639923002/diff/1/src/IceInst.h#newcode79
src/IceInst.h:79: IceString getInstName() const {
On 2016/01/26 19:07:17, John wrote:
> this is really useful, but does not belong in this patch. can you move it to
its
> own cl?

Yup, I'm working on that now.

> 
> This is also going to increase what's called minimal mode (make MINIMAL=1),
> which builds the binary shipped to clients. I would suggest you to add a guard
> 
>   if (!BuildDefs::dump())
>     return "";
> 

Sure.

> I am not sure getInstName() conveys the purpose of this method. It returns the
> instruction **type**, but its name, converted to a string, but I can't think
of
> anything better.

Yeah, I don't really like the name either. What about getKindAsString()? Or
maybe even just toString()?

https://codereview.chromium.org/1639923002/diff/1/src/IceInst.h#newcode79
src/IceInst.h:79: IceString getInstName() const {
On 2016/01/26 19:13:10, stichnot wrote:
> Could you also change the dump() methods in IceInst.cpp to use this method
> instead of hard-coded strings?  This would also mean lower-casing the returned
> strings in this method.

I made some of the obvious changes, but it looks like more generally we might
just be able to get rid of most of the special cases of dump.

https://codereview.chromium.org/1639923002/diff/1/src/IceInst.h#newcode130
src/IceInst.h:130: return "Target";
On 2016/01/26 19:13:10, stichnot wrote:
> Add an assert:
>   assert(Kind >= Target);
> just to make sure that newly added high-level instruction kinds don't forget
to
> add their string here.

Okay.

https://codereview.chromium.org/1639923002/diff/1/src/IceInst.h#newcode346
src/IceInst.h:346: IceString getInstName() const {
On 2016/01/26 19:07:17, John wrote:
> add the
> 
> if (!BuildDefs::dump()) return "";
> 
> and move this to the .cpp file.

Okay.

https://codereview.chromium.org/1639923002/diff/1/src/IceInst.h#newcode346
src/IceInst.h:346: IceString getInstName() const {
On 2016/01/26 19:13:10, stichnot wrote:
> Are you shadowing a nonvirtual method name in a base class?  That seems a
little
> dangerous or error-prone.  Should this be virtual, with override or final
here?

I was thinking the same thing. I made it virtual.

https://codereview.chromium.org/1639923002/diff/1/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/1639923002/diff/1/src/IceTargetLowering.h#new...
src/IceTargetLowering.h:63: llvm_unreachable(StrBuf.str().c_str());             
                    \
On 2016/01/26 19:07:17, John wrote:
> just do
> 
> llvm_unreachable("Not yet implemented: " + Instr->getInstName());

Ok.

[Eric Holk, 2016-01-26 22:56:38]

I removed the UnimplementedLoweringError-related changes and cleaned up the
vector subtraction code based on the review comments.

https://codereview.chromium.org/1639923002/diff/1/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1639923002/diff/1/src/IceInstARM32.cpp#newcod...
src/IceInstARM32.cpp:651: Asm->setNeedsTextFixup();
On 2016/01/26 19:07:17, John wrote:
> remove Asm->setNeedsTextFixup(); and invoke emitUsingTextFixup() here.

Done.

https://codereview.chromium.org/1639923002/diff/1/src/IceInstARM32.cpp#newcod...
src/IceInstARM32.cpp:655: Asm->vsubs(getDest(), getSrc(0), getSrc(1),
CondARM32::AL);
On 2016/01/26 19:07:17, John wrote:
> move the assert below Asm->vsub()

Done.

https://codereview.chromium.org/1639923002/diff/1/src/IceInstARM32.cpp#newcod...
src/IceInstARM32.cpp:662: if (Asm->needsTextFixup()) {
On 2016/01/26 19:07:17, John wrote:
> remove this

Done.

[John, 2016-01-27 15:17:28]

lgtm

[Karl, 2016-01-27 17:58:46]

lgtm

[Eric Holk, 2016-01-27 19:08:51]

Description was changed from

==========
Subzero. ARM32. Vector lowering. Subtract.

This CL also changes UnimplementedLoweringError to display the name of
the unimplemented instruction.

Improve test coverage for ARM32 vector load instructions.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4076
==========

to

==========
Subzero. ARM32. Vector lowering. Subtract.

This CL also changes UnimplementedLoweringError to display the name of
the unimplemented instruction.

Improve test coverage for ARM32 vector load instructions.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4076
R=jpp@chromium.org, kschimpf@google.com

Committed:
https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a...
==========

[Eric Holk, 2016-01-27 19:08:52]

Committed patchset #2 (id:20001) manually as
e5727b836091b786dd069f582aa48452620e600e (presubmit successful).