Start incorporating the ARM integrated assembler.

Start incorporating the ARM integrated assembler.

Extends the ARM32 assembler to be able to generate a trivial function
footprint using the -filetype=iasm option.

Also does a couple of cleanups:

1) Move UnimplementedError macro to common location so that it can be
used by everyone.

2) Add a GlobalContext argument to the assembler, so that it can
look at flags etc.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4334
R=stichnot@chromium.org

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

[Karl, 2015-10-08 22:48:10]

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

[Jim Stichnoth, 2015-10-08 23:50:41]

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler.h
File src/IceAssembler.h (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
src/IceAssembler.h:33: class GlobalContext;
GlobalContext is already forward-declared in IceDefs.h.

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
src/IceAssembler.h:279: // TODO(kschimpf): Remove Ctx argument, no longer
needed.
Can you just do it now, and not leave a TODO?

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
File src/IceAssemblerARM32.h (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
src/IceAssemblerARM32.h:144: // instructions. Based on the "Figure 3-1 ARM
instruction set summary".
Figure 3-1 of what document?

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceTargetLow...
File src/IceTargetLowering.cpp (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceTargetLow...
src/IceTargetLowering.cpp:116: return std::unique_ptr<Assembler>(new
X::Assembler##X(Func->getContext()));
Can you use Ctx instead of Func->getContext() ?

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceTargetLow...
File src/IceTargetLowering.h (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceTargetLow...
src/IceTargetLowering.h:35: if (!static_cast<const ClFlags
&>(Flags).getSkipUnimplemented()) {         \
Do you really need the cast here?

For proper macro safety, you should probably use ((Flags)), though I guess only
one level of parentheses is needed if you drop the cast.

https://chromiumcodereview.appspot.com/1397933002/diff/20001/tests_lit/assemb...
File tests_lit/assembler/arm32/ret.ll (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/tests_lit/assemb...
tests_lit/assembler/arm32/ret.ll:13: ; ASM:      f:
Can this be ASM-LABEL ?
(same with IASM-LABEL below)

[John, 2015-10-09 12:12:24]

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler.h
File src/IceAssembler.h (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
src/IceAssembler.h:33: class GlobalContext;
On 2015/10/08 23:50:41, stichnot wrote:
> GlobalContext is already forward-declared in IceDefs.h.

ah, the beauty of forward decs and cyclic includes!...

I tend to side with Karl here: if we need to forward declare something (i.e.,
Somehow there's a cycle between the #includes in IceGlobalcontext.h and
IceAssembler.h) then that should be done in the file that needs it.

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
src/IceAssembler.h:280: void emitIASBytes(GlobalContext *Ctx) const;
Just a personal preference, but I would not introduce this GlobalContext member.
I hate that (because LLVM Style Guide) parameters, locals, and class members all
look the same. I try to have local declarations (locals and/or parameters) as
much as I can. Feel free to ignore.

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
File src/IceAssemblerARM32.cpp (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
src/IceAssemblerARM32.cpp:56: // TODO(kschimpf) Decide if we have near jumps.
not #if 0 here (and leaving a comment behind) is better.

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
File src/IceAssemblerARM32.h (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
src/IceAssemblerARM32.h:41: explicit AssemblerARM32(GlobalContext *Ctx, bool
use_far_branches = false)
two argument ctor -- explicit is no longer needed. please remove.

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
src/IceAssemblerARM32.h:103: enum {
this trick for introducing class-level constants is no longer needed in C++11.
it's better to do

  static constexpr uint32_t H = 1 << 5;
  static constexpr uint32_t L = 1 << 20;
  static constexpr uint32_t S = 1 << 20;
  ...

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
src/IceAssemblerARM32.h:146: kConditionShift = 28,
This is also just introducing class-level constants...

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
File src/IceAssemblerMIPS32.h (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
src/IceAssemblerMIPS32.h:38: explicit AssemblerMIPS32(GlobalContext *Ctx, bool
use_far_branches = false)
remove explicit.

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
File src/IceAssemblerX8632.h (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
src/IceAssemblerX8632.h:48: explicit AssemblerX8632(GlobalContext *Ctx, bool
use_far_branches = false)
remove explicit.

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
File src/IceAssemblerX8664.h (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
src/IceAssemblerX8664.h:48: explicit AssemblerX8664(GlobalContext *Ctx, bool
use_far_branches = false)
remove explicit.

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
File src/IceAssemblerX86Base.h (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
src/IceAssemblerX86Base.h:118: AssemblerX86Base(AssemblerKind Kind,
GlobalContext *Ctx,
remove explicit.

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceInstARM32...
File src/IceInstARM32.cpp (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceInstARM32...
src/IceInstARM32.cpp:972: ARM32::AssemblerARM32 *Asm =
Func->getAssembler<ARM32::AssemblerARM32>();
hooray!!!! \o/!!!!

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceTargetLow...
File src/IceTargetLowering.h (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceTargetLow...
src/IceTargetLowering.h:35: if (!static_cast<const ClFlags
&>(Flags).getSkipUnimplemented()) {         \
On 2015/10/08 23:50:41, stichnot wrote:
> Do you really need the cast here?
> 
> For proper macro safety, you should probably use ((Flags)), though I guess
only
> one level of parentheses is needed if you drop the cast.

I'll reply on his behalf, for this is my fault.

Regarding the cast, no you don't need it, but it prevents that the macro is used
with anything that exports a getSkipUnimplemented() method. Think "poor man's
type safety."

Regarding the parenthesis, I think

static_cast<>(Flags)

and

static_cast<>((Flags))

are the same. You just wrap macro parameters to prevent associativity issues,
e.g.,

#define TIMES_10(s) (s * 10)

cout << TIMES_10(a + b)  // oops, this is cout << (a + b * 10);

Given that the cast already includes the parenthesis, this should be fine.

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceTargetLow...
File src/IceTargetLoweringARM32.cpp (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceTargetLow...
src/IceTargetLoweringARM32.cpp:3862: case FT_Asm:
hooray 2!!!!

[Jim Stichnoth, 2015-10-09 13:36:30]

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler.h
File src/IceAssembler.h (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
src/IceAssembler.h:33: class GlobalContext;
On 2015/10/09 12:12:23, John wrote:
> On 2015/10/08 23:50:41, stichnot wrote:
> > GlobalContext is already forward-declared in IceDefs.h.
> 
> ah, the beauty of forward decs and cyclic includes!...
> 
> I tend to side with Karl here: if we need to forward declare something (i.e.,
> Somehow there's a cycle between the #includes in IceGlobalcontext.h and
> IceAssembler.h) then that should be done in the file that needs it.

Well, I tried out the patch and everything compiles without this forward
declaration.  Also note the preexisting declaration below:
  void emitIASBytes(GlobalContext *Ctx) const;
So this wasn't a problem that needed fixing.

[Karl, 2015-10-09 19:08:18]

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler.h
File src/IceAssembler.h (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
src/IceAssembler.h:33: class GlobalContext;
On 2015/10/09 13:36:30, stichnot wrote:
> On 2015/10/09 12:12:23, John wrote:
> > On 2015/10/08 23:50:41, stichnot wrote:
> > > GlobalContext is already forward-declared in IceDefs.h.
> > 
> > ah, the beauty of forward decs and cyclic includes!...
> > 
> > I tend to side with Karl here: if we need to forward declare something
(i.e.,
> > Somehow there's a cycle between the #includes in IceGlobalcontext.h and
> > IceAssembler.h) then that should be done in the file that needs it.
> 
> Well, I tried out the patch and everything compiles without this forward
> declaration.  Also note the preexisting declaration below:
>   void emitIASBytes(GlobalContext *Ctx) const;
> So this wasn't a problem that needed fixing.

Removed.

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
src/IceAssembler.h:280: void emitIASBytes(GlobalContext *Ctx) const;
On 2015/10/09 12:12:23, John wrote:
> Just a personal preference, but I would not introduce this GlobalContext
member.
> I hate that (because LLVM Style Guide) parameters, locals, and class members
all
> look the same. I try to have local declarations (locals and/or parameters) as
> much as I can. Feel free to ignore.

While I agree with this comment in general, the global context had to be added
to fix other issues (which would require major changes to many functions to pass
as parameters). Hence, having to pass something that is local appears to
introduce another set of issues. Changing to remove argument Ctx.

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
File src/IceAssemblerARM32.cpp (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
src/IceAssemblerARM32.cpp:56: // TODO(kschimpf) Decide if we have near jumps.
On 2015/10/09 12:12:23, John wrote:
> not #if 0 here (and leaving a comment behind) is better.

Done.

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
File src/IceAssemblerARM32.h (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
src/IceAssemblerARM32.h:41: explicit AssemblerARM32(GlobalContext *Ctx, bool
use_far_branches = false)
On 2015/10/09 12:12:24, John wrote:
> two argument ctor -- explicit is no longer needed. please remove.

Leaving as is, since C++ 11 will otherwise try implicit conversions.

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
src/IceAssemblerARM32.h:103: enum {
On 2015/10/09 12:12:24, John wrote:
> this trick for introducing class-level constants is no longer needed in C++11.
> it's better to do
> 
>   static constexpr uint32_t H = 1 << 5;
>   static constexpr uint32_t L = 1 << 20;
>   static constexpr uint32_t S = 1 << 20;
>   ...

Done.

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
src/IceAssemblerARM32.h:144: // instructions. Based on the "Figure 3-1 ARM
instruction set summary".
On 2015/10/08 23:50:41, stichnot wrote:
> Figure 3-1 of what document?

I agree that this comment is useless (I copied it from Dart, and it doesn't
clarify its context). Replaced with reference to appropriate section in the
ARMv7 manual.

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
src/IceAssemblerARM32.h:146: kConditionShift = 28,
On 2015/10/09 12:12:24, John wrote:
> This is also just introducing class-level constants...

Done.

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
File src/IceAssemblerMIPS32.h (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
src/IceAssemblerMIPS32.h:38: explicit AssemblerMIPS32(GlobalContext *Ctx, bool
use_far_branches = false)
On 2015/10/09 12:12:24, John wrote:
> remove explicit.

Not changing, C++ 11 would then allow implicit conversions.

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
File src/IceAssemblerX8632.h (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
src/IceAssemblerX8632.h:48: explicit AssemblerX8632(GlobalContext *Ctx, bool
use_far_branches = false)
On 2015/10/09 12:12:24, John wrote:
> remove explicit.

Same here.

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
File src/IceAssemblerX8664.h (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
src/IceAssemblerX8664.h:48: explicit AssemblerX8664(GlobalContext *Ctx, bool
use_far_branches = false)
On 2015/10/09 12:12:24, John wrote:
> remove explicit.

Same here.

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
File src/IceAssemblerX86Base.h (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceAssembler...
src/IceAssemblerX86Base.h:118: AssemblerX86Base(AssemblerKind Kind,
GlobalContext *Ctx,
On 2015/10/09 12:12:24, John wrote:
> remove explicit.

Same here.

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceInstARM32...
File src/IceInstARM32.cpp (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceInstARM32...
src/IceInstARM32.cpp:972: ARM32::AssemblerARM32 *Asm =
Func->getAssembler<ARM32::AssemblerARM32>();
On 2015/10/09 12:12:24, John wrote:
> hooray!!!! \o/!!!!

Acknowledged.

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceTargetLow...
File src/IceTargetLowering.cpp (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceTargetLow...
src/IceTargetLowering.cpp:116: return std::unique_ptr<Assembler>(new
X::Assembler##X(Func->getContext()));
On 2015/10/08 23:50:41, stichnot wrote:
> Can you use Ctx instead of Func->getContext() ?

No. This is a static member function.

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceTargetLow...
File src/IceTargetLowering.h (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceTargetLow...
src/IceTargetLowering.h:35: if (!static_cast<const ClFlags
&>(Flags).getSkipUnimplemented()) {         \
On 2015/10/09 12:12:24, John wrote:
> On 2015/10/08 23:50:41, stichnot wrote:
> > Do you really need the cast here?
> > 
> > For proper macro safety, you should probably use ((Flags)), though I guess
> only
> > one level of parentheses is needed if you drop the cast.
> 
> I'll reply on his behalf, for this is my fault.
> 
> Regarding the cast, no you don't need it, but it prevents that the macro is
used
> with anything that exports a getSkipUnimplemented() method. Think "poor man's
> type safety."
> 
> Regarding the parenthesis, I think
> 
> static_cast<>(Flags)
> 
> and
> 
> static_cast<>((Flags))
> 
> are the same. You just wrap macro parameters to prevent associativity issues,
> e.g.,
> 
> #define TIMES_10(s) (s * 10)
> 
> cout << TIMES_10(a + b)  // oops, this is cout << (a + b * 10);
> 
> Given that the cast already includes the parenthesis, this should be fine.

Acknowledged.

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceTargetLow...
File src/IceTargetLoweringARM32.cpp (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceTargetLow...
src/IceTargetLoweringARM32.cpp:3862: case FT_Asm:
On 2015/10/09 12:12:24, John wrote:
> hooray 2!!!!

Acknowledged.

https://chromiumcodereview.appspot.com/1397933002/diff/20001/tests_lit/assemb...
File tests_lit/assembler/arm32/ret.ll (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/tests_lit/assemb...
tests_lit/assembler/arm32/ret.ll:13: ; ASM:      f:
On 2015/10/08 23:50:41, stichnot wrote:
> Can this be ASM-LABEL ?
> (same with IASM-LABEL below)

Done.

[Jim Stichnoth, 2015-10-09 20:05:45]

lgtm

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceTargetLow...
File src/IceTargetLowering.h (right):

https://chromiumcodereview.appspot.com/1397933002/diff/20001/src/IceTargetLow...
src/IceTargetLowering.h:35: if (!static_cast<const ClFlags
&>(Flags).getSkipUnimplemented()) {         \
On 2015/10/09 12:12:24, John wrote:
> On 2015/10/08 23:50:41, stichnot wrote:
> > Do you really need the cast here?
> > 
> > For proper macro safety, you should probably use ((Flags)), though I guess
> only
> > one level of parentheses is needed if you drop the cast.
> 
> I'll reply on his behalf, for this is my fault.
> 
> Regarding the cast, no you don't need it, but it prevents that the macro is
used
> with anything that exports a getSkipUnimplemented() method. Think "poor man's
> type safety."
> 
> Regarding the parenthesis, I think
> 
> static_cast<>(Flags)
> 
> and
> 
> static_cast<>((Flags))
> 
> are the same. You just wrap macro parameters to prevent associativity issues,
> e.g.,
> 
> #define TIMES_10(s) (s * 10)
> 
> cout << TIMES_10(a + b)  // oops, this is cout << (a + b * 10);
> 
> Given that the cast already includes the parenthesis, this should be fine.

I guess I was thinking of the case where the "Flags" macro arg is instantiated
as "A,B" and then substitution makes it parse as two function arguments instead
of a comma expression.  However, I guess you can't really even pass in a comma
in a macro arg unless the whole arg is inside parentheses, so I guess my
argument is moot.

https://chromiumcodereview.appspot.com/1397933002/diff/40001/src/IceAssembler...
File src/IceAssemblerARM32.h (right):

https://chromiumcodereview.appspot.com/1397933002/diff/40001/src/IceAssembler...
src/IceAssemblerARM32.h:156: // Manual, ARMv7-A and ARMv7-R edition"
End sentence with a period.

Also, it would be really nice for the lazy if you could include a URL, even at
the risk of it going stale in the future.

[Karl, 2015-10-09 20:29:17]

Committed patchset #4 (id:60001) manually as
c5abdc1301260c79f60d60ec1117e8044312da30 (presubmit successful).

[Karl, 2015-10-09 20:29:32]

https://chromiumcodereview.appspot.com/1397933002/diff/40001/src/IceAssembler...
File src/IceAssemblerARM32.h (right):

https://chromiumcodereview.appspot.com/1397933002/diff/40001/src/IceAssembler...
src/IceAssemblerARM32.h:156: // Manual, ARMv7-A and ARMv7-R edition"
On 2015/10/09 20:05:45, stichnot wrote:
> End sentence with a period.
> 
> Also, it would be really nice for the lazy if you could include a URL, even at
> the risk of it going stale in the future.

Done.