Subzero: Emit ARM build-attributes in the file scope (as header).

Emit ARM build-attributes in the file scope (as header).

The ARM linker will check that .o files declare compatible
build attributes (e.g., all claim hard-float calling convention,
all claim VFP-vX ,etc.). Thus, in order to set up cross tests that
link LLC generated code against and Subzero generated code,
we need the build attributes to be compatible.

Pick ARMv7, hard-float calling convention, and neon, etc. which
we use for PNaCl LLVM.

Will probably have to reorganize to keep in sync once the ELF
writer also emits this.

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=fb79284d5dd6ea823fa907f3b60015a3d557ce54

[jvoung (off chromium), 2015-06-05 21:58:44]

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

[jvoung (off chromium), 2015-06-05 22:05:54]

https://codereview.chromium.org/1171563002/diff/1/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1171563002/diff/1/src/IceGlobalContext.cpp#ne...
src/IceGlobalContext.cpp:352:
TargetHeaderLowering::createLowering(this)->lower();
Or maybe the flag check and writeInitialELFHeader should happen in the
target-specific "lowering". To both write the ELF header and write the
attributes section.

[John, 2015-06-05 22:28:30]

https://codereview.chromium.org/1171563002/diff/1/src/IceTargetLoweringARM32.h
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1171563002/diff/1/src/IceTargetLoweringARM32....
src/IceTargetLoweringARM32.h:327: class TargetHeaderARM32 : public
TargetHeaderLowering {
Maybe the class should be final, and then you don't have to label each virtual
method as such?

https://codereview.chromium.org/1171563002/diff/1/src/IceTargetLoweringARM32....
src/IceTargetLoweringARM32.h:334: return new TargetHeaderARM32(Ctx);
why not returning a

std::unique_ptr<TargetHeaderLowering> here?

https://codereview.chromium.org/1171563002/diff/1/src/IceTargetLoweringARM32....
src/IceTargetLoweringARM32.h:343: ~TargetHeaderARM32() final {}
= default; instead of {}?

[Karl, 2015-06-08 15:32:26]

lgtm

[Jim Stichnoth, 2015-06-08 20:23:43]

otherwise lgtm

https://codereview.chromium.org/1171563002/diff/1/src/IceTargetLoweringARM32.h
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1171563002/diff/1/src/IceTargetLoweringARM32....
src/IceTargetLoweringARM32.h:334: return new TargetHeaderARM32(Ctx);
On 2015/06/05 22:28:29, John wrote:
> why not returning a
> 
> std::unique_ptr<TargetHeaderLowering> here?

I also think that sounds reasonable, here and for x86-32.  The caller may need
to std::move the result, but that's OK.

https://codereview.chromium.org/1171563002/diff/20001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1171563002/diff/20001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:455: llvm_unreachable("Unsupported target header
lowering");
report_fatal_error

[jvoung (off chromium), 2015-06-08 22:21:10]

Thanks!

https://codereview.chromium.org/1171563002/diff/1/src/IceTargetLoweringARM32.h
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1171563002/diff/1/src/IceTargetLoweringARM32....
src/IceTargetLoweringARM32.h:327: class TargetHeaderARM32 : public
TargetHeaderLowering {
On 2015/06/05 22:28:30, John wrote:
> Maybe the class should be final, and then you don't have to label each virtual
> method as such?

Is that the preferred style?

I could only find "Explicitly annotate overrides of virtual functions or virtual
destructors with an override or (less frequently) final specifier. Older
(pre-C++11) code will use the virtual keyword as an inferior alternative
annotation. For clarity, use exactly one of override, final, or virtual when
declaring an override. Rationale: A function or destructor marked override or
final that is not an override of a base class virtual function will not compile,
and this helps catch common errors. The specifiers serve as documentation; if no
specifier is present, the reader has to check all ancestors of the class in
question to determine if the function or destructor is virtual or not."

Previously we had been marking the methods individually as override or final.
Can marking the class final help find cases where you rename the method in one
part but forget to rename in others?

Otherwise, I could mark the class final if you all prefer that style.

https://codereview.chromium.org/1171563002/diff/1/src/IceTargetLoweringARM32....
src/IceTargetLoweringARM32.h:334: return new TargetHeaderARM32(Ctx);
On 2015/06/08 20:23:43, stichnot wrote:
> On 2015/06/05 22:28:29, John wrote:
> > why not returning a
> > 
> > std::unique_ptr<TargetHeaderLowering> here?
> 
> I also think that sounds reasonable, here and for x86-32.  The caller may need
> to std::move the result, but that's OK.

Done.

https://codereview.chromium.org/1171563002/diff/1/src/IceTargetLoweringARM32....
src/IceTargetLoweringARM32.h:343: ~TargetHeaderARM32() final {}
On 2015/06/05 22:28:30, John wrote:
> = default; instead of {}?

Done.

https://codereview.chromium.org/1171563002/diff/20001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1171563002/diff/20001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:455: llvm_unreachable("Unsupported target header
lowering");
On 2015/06/08 20:23:43, stichnot wrote:
> report_fatal_error

Done.

[John, 2015-06-08 23:22:27]

https://codereview.chromium.org/1171563002/diff/1/src/IceTargetLoweringARM32.h
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1171563002/diff/1/src/IceTargetLoweringARM32....
src/IceTargetLoweringARM32.h:327: class TargetHeaderARM32 : public
TargetHeaderLowering {
On 2015/06/08 22:21:10, jvoung wrote:
> On 2015/06/05 22:28:30, John wrote:
> > Maybe the class should be final, and then you don't have to label each
virtual
> > method as such?
> 
> Is that the preferred style?
> 
> I could only find "Explicitly annotate overrides of virtual functions or
virtual
> destructors with an override or (less frequently) final specifier. Older
> (pre-C++11) code will use the virtual keyword as an inferior alternative
> annotation. For clarity, use exactly one of override, final, or virtual when
> declaring an override. Rationale: A function or destructor marked override or
> final that is not an override of a base class virtual function will not
compile,
> and this helps catch common errors. The specifiers serve as documentation; if
no
> specifier is present, the reader has to check all ancestors of the class in
> question to determine if the function or destructor is virtual or not."
> 
> Previously we had been marking the methods individually as override or final.
> Can marking the class final help find cases where you rename the method in one
> part but forget to rename in others?
> 
> Otherwise, I could mark the class final if you all prefer that style.

There's very little difference in marking a class as final versus each
individual methods. Marking individual methods is useful for it allows more
"modular" final methods. However, marking a whole class as final shows that you
did not intend the class to be derived, which explicitly tells what the
"semantics" for the class are. Final, for methods, are usually used when you
want to "refine" the class behavior. For example:

class Inst {
 ...
 public:
  virtual string EmitAsm() = 0;
 ...
};

class InstIntelX86Return : public Inst {
 public:
  string EmitAsm() final {
    return "\tmov " + ReturnRegister() + ", " + Operand()\n"
           "\tret";
  }

  virtual string ReturnRegister() = 0;
};

class InstIntelX8632Return final : public InstIntelX86Return {
 public:
  string ReturnRegister() {
    return "eax";
  }
};

class InstIntelX8664Return final : public InstIntelX86Return {
 public:
  string ReturnRegister() {
    return "rax";
  }
};

https://codereview.chromium.org/1171563002/diff/60001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1171563002/diff/60001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:451: return
std::unique_ptr<TargetHeaderLowering>(TargetHeader##X::create(Ctx));
you don't need

std::unique_ptr<>

here. Not that it causes any issues -- g++ should be able to effectively
inline-and-optimize-out the move ctors for you. Nonetheless,

return TargetHeader##X::create(Ctx);

should do the trick. Not sure, though...

[jvoung (off chromium), 2015-06-11 22:12:32]

Thanks updated to at compile with MIPS32 target too.

https://codereview.chromium.org/1171563002/diff/1/src/IceTargetLoweringARM32.h
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1171563002/diff/1/src/IceTargetLoweringARM32....
src/IceTargetLoweringARM32.h:327: class TargetHeaderARM32 : public
TargetHeaderLowering {
On 2015/06/08 23:22:26, John wrote:
> On 2015/06/08 22:21:10, jvoung wrote:
> > On 2015/06/05 22:28:30, John wrote:
> > > Maybe the class should be final, and then you don't have to label each
> virtual
> > > method as such?
> > 
> > Is that the preferred style?
> > 
> > I could only find "Explicitly annotate overrides of virtual functions or
> virtual
> > destructors with an override or (less frequently) final specifier. Older
> > (pre-C++11) code will use the virtual keyword as an inferior alternative
> > annotation. For clarity, use exactly one of override, final, or virtual when
> > declaring an override. Rationale: A function or destructor marked override
or
> > final that is not an override of a base class virtual function will not
> compile,
> > and this helps catch common errors. The specifiers serve as documentation;
if
> no
> > specifier is present, the reader has to check all ancestors of the class in
> > question to determine if the function or destructor is virtual or not."
> > 
> > Previously we had been marking the methods individually as override or
final.
> > Can marking the class final help find cases where you rename the method in
one
> > part but forget to rename in others?
> > 
> > Otherwise, I could mark the class final if you all prefer that style.
> 
> There's very little difference in marking a class as final versus each
> individual methods. Marking individual methods is useful for it allows more
> "modular" final methods. However, marking a whole class as final shows that
you
> did not intend the class to be derived, which explicitly tells what the
> "semantics" for the class are. Final, for methods, are usually used when you
> want to "refine" the class behavior. For example:
> 
> class Inst {
>  ...
>  public:
>   virtual string EmitAsm() = 0;
>  ...
> };
> 
> class InstIntelX86Return : public Inst {
>  public:
>   string EmitAsm() final {
>     return "\tmov " + ReturnRegister() + ", " + Operand()\n"
>            "\tret";
>   }
> 
>   virtual string ReturnRegister() = 0;
> };
> 
> class InstIntelX8632Return final : public InstIntelX86Return {
>  public:
>   string ReturnRegister() {
>     return "eax";
>   }
> };
> 
> class InstIntelX8664Return final : public InstIntelX86Return {
>  public:
>   string ReturnRegister() {
>     return "rax";
>   }
> };

Ah, my original worry was if you rename ReturnRegister to ReturnReg, but forget
to do that everywhere. I guess that should at least tell you that you've got an
unimplemented method:

warning: abstract class is marked 'final' [-Wabstract-final-class]
class InstIntelX8664Return final : public InstIntelX86Return {


Looks like Clang at least gives that above error, but GCC 4.8 won't actually
give you an error for the "class C final" version compared to the method final
version. Perhaps later GCC versions has gotten that right in a later version.

Okay, changed to class final.

https://codereview.chromium.org/1171563002/diff/60001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1171563002/diff/60001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:439: #include "llvm/Config/SZTargets.def"
Also updated the MIPS port to have create() return the unique_ptr.

https://codereview.chromium.org/1171563002/diff/60001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:451: return
std::unique_ptr<TargetHeaderLowering>(TargetHeader##X::create(Ctx));
On 2015/06/08 23:22:27, John wrote:
> you don't need
> 
> std::unique_ptr<>
> 
> here. Not that it causes any issues -- g++ should be able to effectively
> inline-and-optimize-out the move ctors for you. Nonetheless,
> 
> return TargetHeader##X::create(Ctx);
> 
> should do the trick. Not sure, though...

Ah right -- Done.

https://codereview.chromium.org/1171563002/diff/80001/src/IceTargetLoweringMI...
File src/IceTargetLoweringMIPS32.h (right):

https://codereview.chromium.org/1171563002/diff/80001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.h:152: class TargetHeaderMIPS32 final : public
TargetHeaderLowering {
added the mips variant without overrides to keep things compiling.

[jvoung (off chromium), 2015-06-11 22:27:56]

Committed patchset #5 (id:80001) manually as
fb79284d5dd6ea823fa907f3b60015a3d557ce54 (presubmit successful).