Subzero: Clean up the runtime implementation.

Subzero: Clean up the runtime implementation.

The runtime helpers are given more consistent names:
  __Sz_<bitcode>_<type1>_<type2>

Missing helpers (various vector bitcasts) are implemented.

We'd prefer to avoid calling external library functions, e.g. in compiler-rt, but even llc uses these helpers for some bitcode on x86-32, so the alternative is to copy their implementation into Subzero's runtime.

BUG= none
R=mtrofin@chromium.org

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

[Jim Stichnoth, 2015-02-27 22:39:34]

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

[Mircea Trofin, 2015-02-28 00:45:35]

https://codereview.chromium.org/961413002/diff/1/runtime/szrt.c
File runtime/szrt.c (right):

https://codereview.chromium.org/961413002/diff/1/runtime/szrt.c#newcode29
runtime/szrt.c:29: uint32_t __Sz_fptoui_f32_i32(float Value) {
What is the motivation for moving to the "__" prefix?

https://codereview.chromium.org/961413002/diff/1/src/IceTargetLoweringX8632.cpp
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/961413002/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:2101: ? "__Sz_fptosi_f32_i64"
Do we expect to use these strings anywhere else, should we define constant
strings for them?

[Jim Stichnoth, 2015-02-28 02:35:44]

https://codereview.chromium.org/961413002/diff/1/runtime/szrt.c
File runtime/szrt.c (right):

https://codereview.chromium.org/961413002/diff/1/runtime/szrt.c#newcode29
runtime/szrt.c:29: uint32_t __Sz_fptoui_f32_i32(float Value) {
On 2015/02/28 00:45:35, Mircea Trofin wrote:
> What is the motivation for moving to the "__" prefix?

As I understand it, C reserves the two-underscore prefix for the compiler
libraries, in order to avoid name conflicts.  I'm not sure of the official
reference, but there is a statement here:
http://www.gnu.org/software/libc/manual/html_node/Reserved-Names.html

Alternate suggestions welcome though.

https://codereview.chromium.org/961413002/diff/1/src/IceTargetLoweringX8632.cpp
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/961413002/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:2101: ? "__Sz_fptosi_f32_i64"
On 2015/02/28 00:45:35, Mircea Trofin wrote:
> Do we expect to use these strings anywhere else, should we define constant
> strings for them?

Good point.  It's likely that these and more will used by other targets, so I
moved them into the TargetLowering base class.

I'm not sure whether to declare them as char* or IceString.  If I use char*, I
can probably put the entire definition in the .h file.  If I use IceString,
startup may be slower but the use during target lowering may be faster.

[Mircea Trofin, 2015-02-28 03:28:57]

lgtm

https://codereview.chromium.org/961413002/diff/1/runtime/szrt.c
File runtime/szrt.c (right):

https://codereview.chromium.org/961413002/diff/1/runtime/szrt.c#newcode29
runtime/szrt.c:29: uint32_t __Sz_fptoui_f32_i32(float Value) {
On 2015/02/28 02:35:44, stichnot wrote:
> On 2015/02/28 00:45:35, Mircea Trofin wrote:
> > What is the motivation for moving to the "__" prefix?
> 
> As I understand it, C reserves the two-underscore prefix for the compiler
> libraries, in order to avoid name conflicts.  I'm not sure of the official
> reference, but there is a statement here:
> http://www.gnu.org/software/libc/manual/html_node/Reserved-Names.html
> 
> Alternate suggestions welcome though.

I see - thanks for the clarification!

https://codereview.chromium.org/961413002/diff/1/src/IceTargetLoweringX8632.cpp
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/961413002/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:2101: ? "__Sz_fptosi_f32_i64"
On 2015/02/28 02:35:44, stichnot wrote:
> On 2015/02/28 00:45:35, Mircea Trofin wrote:
> > Do we expect to use these strings anywhere else, should we define constant
> > strings for them?
> 
> Good point.  It's likely that these and more will used by other targets, so I
> moved them into the TargetLowering base class.
> 
> I'm not sure whether to declare them as char* or IceString.  If I use char*, I
> can probably put the entire definition in the .h file.  If I use IceString,
> startup may be slower but the use during target lowering may be faster.

Why would the use during lowering be faster?

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

https://codereview.chromium.org/961413002/diff/20001/src/IceTargetLowering.cp...
src/IceTargetLowering.cpp:29: 
(overly-pedantic, but...) if the const values lived in their own module, one
could link just that module without the overhead of the rest of this current
one. Now, I can't really articulate a rationale one would use the symbols but
not the rest of the library - and we can always refactor, I suppose.

[Mircea Trofin, 2015-02-28 03:28:58]

lgtm

[Jim Stichnoth, 2015-02-28 04:44:29]

PTAL

https://codereview.chromium.org/961413002/diff/1/runtime/szrt.c
File runtime/szrt.c (right):

https://codereview.chromium.org/961413002/diff/1/runtime/szrt.c#newcode29
runtime/szrt.c:29: uint32_t __Sz_fptoui_f32_i32(float Value) {
On 2015/02/28 03:28:56, Mircea Trofin wrote:
> On 2015/02/28 02:35:44, stichnot wrote:
> > On 2015/02/28 00:45:35, Mircea Trofin wrote:
> > > What is the motivation for moving to the "__" prefix?
> > 
> > As I understand it, C reserves the two-underscore prefix for the compiler
> > libraries, in order to avoid name conflicts.  I'm not sure of the official
> > reference, but there is a statement here:
> > http://www.gnu.org/software/libc/manual/html_node/Reserved-Names.html
> > 
> > Alternate suggestions welcome though.
> 
> I see - thanks for the clarification!

Acknowledged.

https://codereview.chromium.org/961413002/diff/1/src/IceTargetLoweringX8632.cpp
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/961413002/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:2101: ? "__Sz_fptosi_f32_i64"
On 2015/02/28 03:28:56, Mircea Trofin wrote:
> On 2015/02/28 02:35:44, stichnot wrote:
> > On 2015/02/28 00:45:35, Mircea Trofin wrote:
> > > Do we expect to use these strings anywhere else, should we define constant
> > > strings for them?
> > 
> > Good point.  It's likely that these and more will used by other targets, so
I
> > moved them into the TargetLowering base class.
> > 
> > I'm not sure whether to declare them as char* or IceString.  If I use char*,
I
> > can probably put the entire definition in the .h file.  If I use IceString,
> > startup may be slower but the use during target lowering may be faster.
> 
> Why would the use during lowering be faster?

I'm assuming that copying a constant IceString (i.e. std::string) during target
lowering will be just a shallow copy operation, while constructing a new
IceString from a const char* may copy all the chars.  The tradeoff is that
static IceStrings mean static initializers, which may be banned in Chrome (I
can't find a reference, but
http://neugierig.org/software/chromium/notes/2011/08/static-initializers.html is
interesting reading).

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

https://codereview.chromium.org/961413002/diff/20001/src/IceTargetLowering.cp...
src/IceTargetLowering.cpp:29: 
On 2015/02/28 03:28:57, Mircea Trofin wrote:
> (overly-pedantic, but...) if the const values lived in their own module, one
> could link just that module without the overhead of the rest of this current
> one. Now, I can't really articulate a rationale one would use the symbols but
> not the rest of the library - and we can always refactor, I suppose.

Thinking about this further - when we add a new target, it may require new
helpers that the old target didn't need, and it may not use all the helpers that
the old target needed.  I suspect that when building pnacl-sz for a target using
-ffunction-sections -fdata-sections, unused static char* variables would be
removed but not statically initialized but subsequently unused IceString
variables would not.

With all that in mind, I think I'll go with the const char* approach instead.

[Mircea Trofin, 2015-02-28 06:50:20]

On 2015/02/28 04:44:29, stichnot wrote:
> PTAL
> 
> https://codereview.chromium.org/961413002/diff/1/runtime/szrt.c
> File runtime/szrt.c (right):
> 
> https://codereview.chromium.org/961413002/diff/1/runtime/szrt.c#newcode29
> runtime/szrt.c:29: uint32_t __Sz_fptoui_f32_i32(float Value) {
> On 2015/02/28 03:28:56, Mircea Trofin wrote:
> > On 2015/02/28 02:35:44, stichnot wrote:
> > > On 2015/02/28 00:45:35, Mircea Trofin wrote:
> > > > What is the motivation for moving to the "__" prefix?
> > > 
> > > As I understand it, C reserves the two-underscore prefix for the compiler
> > > libraries, in order to avoid name conflicts.  I'm not sure of the official
> > > reference, but there is a statement here:
> > > http://www.gnu.org/software/libc/manual/html_node/Reserved-Names.html
> > > 
> > > Alternate suggestions welcome though.
> > 
> > I see - thanks for the clarification!
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/961413002/diff/1/src/IceTargetLoweringX8632.cpp
> File src/IceTargetLoweringX8632.cpp (right):
> 
>
https://codereview.chromium.org/961413002/diff/1/src/IceTargetLoweringX8632.c...
> src/IceTargetLoweringX8632.cpp:2101: ? "__Sz_fptosi_f32_i64"
> On 2015/02/28 03:28:56, Mircea Trofin wrote:
> > On 2015/02/28 02:35:44, stichnot wrote:
> > > On 2015/02/28 00:45:35, Mircea Trofin wrote:
> > > > Do we expect to use these strings anywhere else, should we define
constant
> > > > strings for them?
> > > 
> > > Good point.  It's likely that these and more will used by other targets,
so
> I
> > > moved them into the TargetLowering base class.
> > > 
> > > I'm not sure whether to declare them as char* or IceString.  If I use
char*,
> I
> > > can probably put the entire definition in the .h file.  If I use
IceString,
> > > startup may be slower but the use during target lowering may be faster.
> > 
> > Why would the use during lowering be faster?
> 
> I'm assuming that copying a constant IceString (i.e. std::string) during
target
> lowering will be just a shallow copy operation, while constructing a new
> IceString from a const char* may copy all the chars.  The tradeoff is that
> static IceStrings mean static initializers, which may be banned in Chrome (I
> can't find a reference, but
> http://neugierig.org/software/chromium/notes/2011/08/static-initializers.html
is
> interesting reading).
> 
> https://codereview.chromium.org/961413002/diff/20001/src/IceTargetLowering.cpp
> File src/IceTargetLowering.cpp (right):
> 
>
https://codereview.chromium.org/961413002/diff/20001/src/IceTargetLowering.cp...
> src/IceTargetLowering.cpp:29: 
> On 2015/02/28 03:28:57, Mircea Trofin wrote:
> > (overly-pedantic, but...) if the const values lived in their own module, one
> > could link just that module without the overhead of the rest of this current
> > one. Now, I can't really articulate a rationale one would use the symbols
but
> > not the rest of the library - and we can always refactor, I suppose.
> 
> Thinking about this further - when we add a new target, it may require new
> helpers that the old target didn't need, and it may not use all the helpers
that
> the old target needed.  I suspect that when building pnacl-sz for a target
using
> -ffunction-sections -fdata-sections, unused static char* variables would be
> removed but not statically initialized but subsequently unused IceString
> variables would not.
> 
> With all that in mind, I think I'll go with the const char* approach instead.

LGTM

[Jim Stichnoth, 2015-03-02 07:13:00]

Committed patchset #3 (id:40001) manually as
c45087913705fe19eca05c321c240fed6058343f (presubmit successful).