Refactor PUSH/POP in ARM assemblers.

Refactor PUSH/POP in ARM assemblers.

Refactors methods emit() and emitIAS() of InstARM32Push and InstARM32Pop
to separate out the selection of assembler instructions from instruction
emission, using template methods.

Template method assemble() provides a single implementation for emit() and emitIAS(). This method calls template functions in the assembler to generate textual and binary forms of the instruction.

BUG= None
R=jpp@chromium.org, stichnot@chromium.org

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

[Karl, 2015-12-18 20:01:15]

Description was changed from

==========
Refactor PUSH/POP in ARM assemblers.

Refactors methods emit() and emitIAS() of InstARM32Push and InstARM32Pop
to separate out the selection of assembler instructions from instruction
emission, using template methods.

Template method assemble() provides a single implementation for emit() and
emitIAS(). This method calls template functions in the assembler to generate
textual and binary forms of the instruction.

BUG= None
==========

to

==========
Refactor PUSH/POP in ARM assemblers.

Refactors methods emit() and emitIAS() of InstARM32Push and InstARM32Pop
to separate out the selection of assembler instructions from instruction
emission, using template methods.

Template method assemble() provides a single implementation for emit() and
emitIAS(). This method calls template functions in the assembler to generate
textual and binary forms of the instruction.

BUG= None
==========

[Karl, 2015-12-18 20:01:15]

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

[John, 2015-12-18 20:22:27]

I can't fully review this now, but please wait for my review before landing

[John, 2016-01-04 15:27:49]

Getting rid of duplication is always nice, but this CL does something else also:
it moves logic from Inst to the Assembler.

In Subzero, each target lowering has three layers

+----------------+
| TargetLowering |
+----------------+
| Inst           |
+----------------+
| Assembler      |
+----------------+

that cooperate with each other for code emission.

The TargetLowering knows about which operations are possible in the target
processor. For example, the ARM32 backend knows that ARM processors can

add r0, r1, r2

or

add r0, r1, r2, <<shift>> r3

or

add r0, r1, r2, <<shift>> #const

or

add r0, r1, #const

so it emits all of those variants.

The Assembler layer knows how to encode instructions. It should have minimal
error handling (mostly, it should abort the whole process if the TargetLowering
selected an invalid operand/instruction combination), and nothing else.

The Inst layer is the glue between these two layers. All the logic that don't
really belong in the lowering, but is too complicated for the assembler, should
live here. The push/pop emission is a good example of logic that does not belong
in the assembler.

I really don't think the assembler should be modified to handle textual output.
If you want to eliminate duplication, the refactoring should not cross the inst
layer. Or the target lowering should be modified to only emit pushes/pops with
valid operands.

Maybe the other stakeholders should also pitch in.

https://codereview.chromium.org/1535233002/diff/40001/src/IceAssembler.h
File src/IceAssembler.h (right):

https://codereview.chromium.org/1535233002/diff/40001/src/IceAssembler.h#newc...
src/IceAssembler.h:37: // Defines form that assembly instruction should be
synthesized.
This is ARM specific. Please so not add it to target-independent headers.

https://codereview.chromium.org/1535233002/diff/40001/src/IceAssemblerARM32.h
File src/IceAssemblerARM32.h (right):

https://codereview.chromium.org/1535233002/diff/40001/src/IceAssemblerARM32.h...
src/IceAssemblerARM32.h:258: template <AssemblyOutputForm>
The idea of the assembler emitting textual output is very confusing. Plus, this
feature is only available for select instructions, which makes matters even more
confusing. I would say no assembler should emit textual output.

[Karl, 2016-01-05 17:43:03]

Refactored code as suggested.

https://codereview.chromium.org/1535233002/diff/40001/src/IceAssembler.h
File src/IceAssembler.h (right):

https://codereview.chromium.org/1535233002/diff/40001/src/IceAssembler.h#newc...
src/IceAssembler.h:37: // Defines form that assembly instruction should be
synthesized.
On 2016/01/04 15:27:49, John wrote:
> This is ARM specific. Please so not add it to target-independent headers.

Moved to IceInstARM32.h

https://codereview.chromium.org/1535233002/diff/40001/src/IceAssemblerARM32.h
File src/IceAssemblerARM32.h (right):

https://codereview.chromium.org/1535233002/diff/40001/src/IceAssemblerARM32.h...
src/IceAssemblerARM32.h:258: template <AssemblyOutputForm>
On 2016/01/04 15:27:49, John wrote:
> The idea of the assembler emitting textual output is very confusing. Plus,
this
> feature is only available for select instructions, which makes matters even
more
> confusing. I would say no assembler should emit textual output.

Refactored so that textual generation is only in IceInstARM32.cpp.

[John, 2016-01-06 16:10:21]

https://codereview.chromium.org/1535233002/diff/80001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1535233002/diff/80001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:534: assert(Var->hasReg());
optional: This assertion is not needed -- Var->getRegNum() will assert for you.

https://codereview.chromium.org/1535233002/diff/80001/src/IceAssemblerARM32.h
File src/IceAssemblerARM32.h (right):

https://codereview.chromium.org/1535233002/diff/80001/src/IceAssemblerARM32.h...
src/IceAssemblerARM32.h:108: static IValueT getEncodedGPRegNum(const Variable
*Var);
can these be moved to anonymous namespaces within the .cc file?

https://codereview.chromium.org/1535233002/diff/80001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1535233002/diff/80001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:99: void
InstARM32::startNextAsmInst<InstARM32::BinaryOutput>(
The fact that this does nothing tells me this method is not really a template.
You could simply leave as a non-templated method, and only invoke it before
returning on

void InstARM32Pop::emitPop<InstARM32::TextualOutput>()
void InstARM32Pop::emitPopList<InstARM32::TextualOutput>()
void InstARM32Push::emitPush<InstARM32::BinaryOutput>()
void InstARM32Push::emitPushList<InstARM32::TextualOutput>()

https://codereview.chromium.org/1535233002/diff/80001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1395: IValueT Base =
AssemblerARM32::getEncodedSRegNum(BaseReg);
Why using this method? RegARM32::getEncodedSReg() returns the same thing, with
the advantage that you don't have to move private, assembler-only routines to
the public interface.

https://codereview.chromium.org/1535233002/diff/80001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1494: assemble<TextualOutput>(Func);
I would rather have the

if (!BuildDefs::dump()) {
  return;
}

here. Otherwise, I have to look at assemble<TextualOutput>() to realize this
method will not expand in minimal build.

https://codereview.chromium.org/1535233002/diff/80001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1521: this->emitPush(Func);
do you need the

this->

?

https://codereview.chromium.org/1535233002/diff/80001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1556: IValueT Base =
AssemblerARM32::getEncodedSRegNum(BaseReg);
ditto.

https://codereview.chromium.org/1535233002/diff/80001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1576: void InstARM32Push::assemble(const Cfg *Func) const {
assemble is not really meaningful for textual output. how about naming this
emitCommon?

https://codereview.chromium.org/1535233002/diff/80001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1605: return;
You should llvm::report_fatal_error() here. It is known that there's at least 1
source, and that that source is an integer Variable. This code path should
really never be reached.

https://codereview.chromium.org/1535233002/diff/80001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1535233002/diff/80001/src/IceInstARM32.h#newc...
src/IceInstARM32.h:1071: class InstARM32Pop : public InstARM32 {
Push and pop have nearly-identical emitters. It seems like there should be a

class InstARM32RegisterSaveRegister : public InstARM32 {
protected:
  void Assemble(OutputForm);
  virtual void emitSingle(OutputForm, Variable *Variable) = 0;
  virtual void emitMultiple(OutputForm, RegisterList) = 0;
};

class InstARM32Pop : public InstARM32RegisterSaveRegister {
protected:
  void emitSingle(OutputForm, Variable *Variable) override {
    switch (OutputForm) {
      case Binary:
        Asm->pop(...);
        return;
      case Textual: {
        if (!BuildDefs::dump())
          return;
        inline emitPop() here.
      }
    }
  }
}

class InstARM32Push : public InstARM32RegisterSaveRegister {}

This also gets rid of the template functions. Templates would definitively be
more efficient here, but the non-templated-code is easier to follow.

https://codereview.chromium.org/1535233002/diff/80001/src/IceRegistersARM32.h
File src/IceRegistersARM32.h (right):

https://codereview.chromium.org/1535233002/diff/80001/src/IceRegistersARM32.h...
src/IceRegistersARM32.h:159: static inline IceString getGPRegName(SizeT RegNum)
{
All throughout this file GPRs are named GPRs, not GPReg. I would like naming to
be consistent.

[Karl, 2016-01-06 23:21:58]

https://codereview.chromium.org/1535233002/diff/80001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1535233002/diff/80001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:534: assert(Var->hasReg());
On 2016/01/06 16:10:21, John wrote:
> optional: This assertion is not needed -- Var->getRegNum() will assert for
you.

Done.

https://codereview.chromium.org/1535233002/diff/80001/src/IceAssemblerARM32.h
File src/IceAssemblerARM32.h (right):

https://codereview.chromium.org/1535233002/diff/80001/src/IceAssemblerARM32.h...
src/IceAssemblerARM32.h:108: static IValueT getEncodedGPRegNum(const Variable
*Var);
On 2016/01/06 16:10:21, John wrote:
> can these be moved to anonymous namespaces within the .cc file?

Moving back to anonymous namespace.

https://codereview.chromium.org/1535233002/diff/80001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1535233002/diff/80001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:99: void
InstARM32::startNextAsmInst<InstARM32::BinaryOutput>(
On 2016/01/06 16:10:21, John wrote:
> The fact that this does nothing tells me this method is not really a template.
> You could simply leave as a non-templated method, and only invoke it before
> returning on
> 
> void InstARM32Pop::emitPop<InstARM32::TextualOutput>()
> void InstARM32Pop::emitPopList<InstARM32::TextualOutput>()
> void InstARM32Push::emitPush<InstARM32::BinaryOutput>()
> void InstARM32Push::emitPushList<InstARM32::TextualOutput>()

Removed.

https://codereview.chromium.org/1535233002/diff/80001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1395: IValueT Base =
AssemblerARM32::getEncodedSRegNum(BaseReg);
On 2016/01/06 16:10:21, John wrote:
> Why using this method? RegARM32::getEncodedSReg() returns the same thing, with
> the advantage that you don't have to move private, assembler-only routines to
> the public interface.

Fixed.

https://codereview.chromium.org/1535233002/diff/80001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1494: assemble<TextualOutput>(Func);
On 2016/01/06 16:10:21, John wrote:
> I would rather have the
> 
> if (!BuildDefs::dump()) {
>   return;
> }
> 
> here. Otherwise, I have to look at assemble<TextualOutput>() to realize this
> method will not expand in minimal build.

Done.

https://codereview.chromium.org/1535233002/diff/80001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1521: this->emitPush(Func);
On 2016/01/06 16:10:21, John wrote:
> do you need the
> 
> this->
> 
> ?

Removed.

https://codereview.chromium.org/1535233002/diff/80001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1556: IValueT Base =
AssemblerARM32::getEncodedSRegNum(BaseReg);
On 2016/01/06 16:10:21, John wrote:
> ditto.

Done.

https://codereview.chromium.org/1535233002/diff/80001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1576: void InstARM32Push::assemble(const Cfg *Func) const {
On 2016/01/06 16:10:21, John wrote:
> assemble is not really meaningful for textual output. how about naming this
> emitCommon?

renamed to emitUsingForm().

https://codereview.chromium.org/1535233002/diff/80001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1605: return;
On 2016/01/06 16:10:21, John wrote:
> You should llvm::report_fatal_error() here. It is known that there's at least
1
> source, and that that source is an integer Variable. This code path should
> really never be reached.

Actually, this case can't happen due to the test "if (SrcSize == 0)", so
removing.

https://codereview.chromium.org/1535233002/diff/80001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1535233002/diff/80001/src/IceInstARM32.h#newc...
src/IceInstARM32.h:1071: class InstARM32Pop : public InstARM32 {
On 2016/01/06 16:10:21, John wrote:
> Push and pop have nearly-identical emitters. It seems like there should be a
> 
> class InstARM32RegisterSaveRegister : public InstARM32 {
> protected:
>   void Assemble(OutputForm);
>   virtual void emitSingle(OutputForm, Variable *Variable) = 0;
>   virtual void emitMultiple(OutputForm, RegisterList) = 0;
> };
> 
> class InstARM32Pop : public InstARM32RegisterSaveRegister {
> protected:
>   void emitSingle(OutputForm, Variable *Variable) override {
>     switch (OutputForm) {
>       case Binary:
>         Asm->pop(...);
>         return;
>       case Textual: {
>         if (!BuildDefs::dump())
>           return;
>         inline emitPop() here.
>       }
>     }
>   }
> }
> 
> class InstARM32Push : public InstARM32RegisterSaveRegister {}
> 
> This also gets rid of the template functions. Templates would definitively be
> more efficient here, but the non-templated-code is easier to follow.

Rewrote with base class and virtuals.

https://codereview.chromium.org/1535233002/diff/80001/src/IceRegistersARM32.h
File src/IceRegistersARM32.h (right):

https://codereview.chromium.org/1535233002/diff/80001/src/IceRegistersARM32.h...
src/IceRegistersARM32.h:159: static inline IceString getGPRegName(SizeT RegNum)
{
On 2016/01/06 16:10:21, John wrote:
> All throughout this file GPRs are named GPRs, not GPReg. I would like naming
to
> be consistent.

Done.

[Karl, 2016-01-08 15:21:38]

Ping?

[Jim Stichnoth, 2016-01-10 18:22:00]

Otherwise lgtm.

It would be good to get John's signoff as well before landing.

https://codereview.chromium.org/1535233002/diff/120001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1535233002/diff/120001/src/IceInstARM32.h#new...
src/IceInstARM32.h:374: enum OutputForm { TextualOutput, BinaryOutput };
My preference (as littered throughout the code base) for these constexpr enum
values is to start the names with some short prefix plus an underscore, e.g.
  enum OutputForm { OF_TextualOutput, OF_BinaryOutput };

https://codereview.chromium.org/1535233002/diff/120001/src/IceRegistersARM32.h
File src/IceRegistersARM32.h (right):

https://codereview.chromium.org/1535233002/diff/120001/src/IceRegistersARM32....
src/IceRegistersARM32.h:166: assert(RegNum <= getNumGPRs());
Should this be "<" instead of "<=" ?

(same comment for below)

[John, 2016-01-11 13:40:36]

lgtm

https://codereview.chromium.org/1535233002/diff/120001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1535233002/diff/120001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:676: << "v" << getOpcode() << "\t{";
optional: I am not a big fan of this strategy. I personally would prefer if the
strings "vpop", and "vpush" was somewhere in this file.

https://codereview.chromium.org/1535233002/diff/120001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1535233002/diff/120001/src/IceInstARM32.h#new...
src/IceInstARM32.h:374: enum OutputForm { TextualOutput, BinaryOutput };
On 2016/01/10 18:21:59, stichnot wrote:
> My preference (as littered throughout the code base) for these constexpr enum
> values is to start the names with some short prefix plus an underscore, e.g.
>   enum OutputForm { OF_TextualOutput, OF_BinaryOutput };

I am not a fan of these prefixes myself (they are hard to remember, and convey
little semantic benefit to justify adding them), but given the nonsensical llvm
style guide there's no better way to differentiate between a variable and an
enum value.

https://codereview.chromium.org/1535233002/diff/120001/src/IceRegistersARM32.h
File src/IceRegistersARM32.h (right):

https://codereview.chromium.org/1535233002/diff/120001/src/IceRegistersARM32....
src/IceRegistersARM32.h:155: static inline SizeT getNumGPRs() {
You don't need this method if you use Table[] -- see the comment below.

https://codereview.chromium.org/1535233002/diff/120001/src/IceRegistersARM32....
src/IceRegistersARM32.h:166: assert(RegNum <= getNumGPRs());
On 2016/01/10 18:22:00, stichnot wrote:
> Should this be "<" instead of "<=" ?
> 
> (same comment for below)

Now that we have RegARM32::Table[], this should be implemented (IMO) as:

  assert(RegNum < Reg_NUM);
  assert(Table[RegNum].IsGPR);
  return Table[RegNum].Name;

(all of these functions should be re-defined to use Table[], but that should be
done in a separate CL)

https://codereview.chromium.org/1535233002/diff/120001/src/IceRegistersARM32....
src/IceRegistersARM32.h:190: static inline SizeT getNumSRegs() {
can you remove inline here, and add constexpr?

static constexpr SizeT getNumSRegs()

?

https://codereview.chromium.org/1535233002/diff/120001/src/IceRegistersARM32....
src/IceRegistersARM32.h:201: assert(RegNum <= getNumSRegs());
Please use Table[].

  assert(RegNum < Reg_NUM);
  assert(Table[RegNum].IsFP32);
  return Table[RegNum].Name;

[Karl, 2016-01-11 17:59:25]

Description was changed from

==========
Refactor PUSH/POP in ARM assemblers.

Refactors methods emit() and emitIAS() of InstARM32Push and InstARM32Pop
to separate out the selection of assembler instructions from instruction
emission, using template methods.

Template method assemble() provides a single implementation for emit() and
emitIAS(). This method calls template functions in the assembler to generate
textual and binary forms of the instruction.

BUG= None
==========

to

==========
Refactor PUSH/POP in ARM assemblers.

Refactors methods emit() and emitIAS() of InstARM32Push and InstARM32Pop
to separate out the selection of assembler instructions from instruction
emission, using template methods.

Template method assemble() provides a single implementation for emit() and
emitIAS(). This method calls template functions in the assembler to generate
textual and binary forms of the instruction.

BUG= None
R=jpp@chromium.org, stichnot@chromium.org

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

[Karl, 2016-01-11 17:59:26]

Committed patchset #8 (id:140001) manually as
c411dbf1c868911d4d300c6bf1ca6caa62592a1c (presubmit successful).

[Karl, 2016-01-11 17:59:44]

https://codereview.chromium.org/1535233002/diff/120001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1535233002/diff/120001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:676: << "v" << getOpcode() << "\t{";
On 2016/01/11 13:40:36, John wrote:
> optional: I am not a big fan of this strategy. I personally would prefer if
the
> strings "vpop", and "vpush" was somewhere in this file. 

Fixed by adding virtual to differentiate GPR opcodes and SReg opcodes, and
adding text strings to the corresponding virtual.

https://codereview.chromium.org/1535233002/diff/120001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1535233002/diff/120001/src/IceInstARM32.h#new...
src/IceInstARM32.h:374: enum OutputForm { TextualOutput, BinaryOutput };
On 2016/01/10 18:21:59, stichnot wrote:
> My preference (as littered throughout the code base) for these constexpr enum
> values is to start the names with some short prefix plus an underscore, e.g.
>   enum OutputForm { OF_TextualOutput, OF_BinaryOutput };

Done.

https://codereview.chromium.org/1535233002/diff/120001/src/IceInstARM32.h#new...
src/IceInstARM32.h:374: enum OutputForm { TextualOutput, BinaryOutput };
On 2016/01/11 13:40:36, John wrote:
> On 2016/01/10 18:21:59, stichnot wrote:
> > My preference (as littered throughout the code base) for these constexpr
enum
> > values is to start the names with some short prefix plus an underscore, e.g.
> >   enum OutputForm { OF_TextualOutput, OF_BinaryOutput };
> 
> I am not a fan of these prefixes myself (they are hard to remember, and convey
> little semantic benefit to justify adding them), but given the nonsensical
llvm
> style guide there's no better way to differentiate between a variable and an
> enum value.

Acknowledged.

https://codereview.chromium.org/1535233002/diff/120001/src/IceRegistersARM32.h
File src/IceRegistersARM32.h (right):

https://codereview.chromium.org/1535233002/diff/120001/src/IceRegistersARM32....
src/IceRegistersARM32.h:155: static inline SizeT getNumGPRs() {
On 2016/01/11 13:40:36, John wrote:
> You don't need this method if you use Table[] -- see the comment below.

This form is used because it allows it to be a constexpr, and using local
variables (to do the counting) is not allowed till c++ 14.

https://codereview.chromium.org/1535233002/diff/120001/src/IceRegistersARM32....
src/IceRegistersARM32.h:166: assert(RegNum <= getNumGPRs());
On 2016/01/10 18:22:00, stichnot wrote:
> Should this be "<" instead of "<=" ?
> 
> (same comment for below)

Done.

https://codereview.chromium.org/1535233002/diff/120001/src/IceRegistersARM32....
src/IceRegistersARM32.h:166: assert(RegNum <= getNumGPRs());
On 2016/01/10 18:22:00, stichnot wrote:
> Should this be "<" instead of "<=" ?
> 
> (same comment for below)

As far as John's comment, I was making too many assumptions. Simplifying.

Also, I agree that the rest of the file should be cleaned up to use the table.

https://codereview.chromium.org/1535233002/diff/120001/src/IceRegistersARM32....
src/IceRegistersARM32.h:190: static inline SizeT getNumSRegs() {
On 2016/01/11 13:40:36, John wrote:
> can you remove inline here, and add constexpr?
> 
> static constexpr SizeT getNumSRegs()
> 
> ?

Done.

https://codereview.chromium.org/1535233002/diff/120001/src/IceRegistersARM32....
src/IceRegistersARM32.h:201: assert(RegNum <= getNumSRegs());
On 2016/01/11 13:40:36, John wrote:
> Please use Table[].
> 
>   assert(RegNum < Reg_NUM);
>   assert(Table[RegNum].IsFP32);
>   return Table[RegNum].Name;

Done.