Cache common constants before lowering.

Cache common constants before lowering.

This code moves some constants used by the target lowering, so that they
are defined (and cached) during static initialization of the target
lowering, rather than looking up everytime they are used.

This CL does this for the constant zero, and predefined helper functions.

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

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

[Karl, 2016-03-09 19:05:04]

Description was changed from

==========
Cache common constants before lowering.

This code moves some constants used by the target lowering, so that they
are defined (and cached) during static initialization of the target
lowering, rather than looking up everytime they are used.

This CL does this for the constant zero, and predefined helper functions.

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

to

==========
Cache common constants before lowering.

This code moves some constants used by the target lowering, so that they
are defined (and cached) during static initialization of the target
lowering, rather than looking up everytime they are used.

This CL does this for the constant zero, and predefined helper functions.

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

[Karl, 2016-03-09 20:01:10]

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

[John, 2016-03-09 23:26:56]

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

https://codereview.chromium.org/1775253003/diff/20001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:265: #define X(Ty)                                    
                             \
This looks dangerous in the context of the browser translator - when Ctx is
deleted the array will point to bad data. I think this array should be a
non-static member in global context.

https://codereview.chromium.org/1775253003/diff/20001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:266: ConstZeroForType[IceType_##Ty] =
Ctx->getConstantZero(IceType_##Ty);
It would be better if this used ICETYPE_TABLE... You could add a field to those
that is set to 1 if the type should have a constant zero

https://codereview.chromium.org/1775253003/diff/20001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/1775253003/diff/20001/src/IceTargetLowering.h...
src/IceTargetLowering.h:600: static Constant *ConstZeroForType[IceType_NUM];
What do you think of making this array a non-static member of global context?
Constants are tied to global context anyways.

[Jim Stichnoth, 2016-03-10 00:24:46]

https://codereview.chromium.org/1775253003/diff/20001/src/IceTargetLowering.def
File src/IceTargetLowering.def (right):

https://codereview.chromium.org/1775253003/diff/20001/src/IceTargetLowering.d...
src/IceTargetLowering.def:34: X(bitcast_16xi1_i16, "__Sz_bitcast_16xi1_i16")    
                          \
Could you reformat this into aligned columns?

https://codereview.chromium.org/1775253003/diff/20001/src/IceTargetLowering.d...
src/IceTargetLowering.def:72: 
remove one blank line

https://codereview.chromium.org/1775253003/diff/20001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/1775253003/diff/20001/src/IceTargetLowering.h...
src/IceTargetLowering.h:580: /// Returns constant zero of Ty.
You should probably also try to capture the calls to
Ctx->getConstantInt{1,8,16,32,64}(0).  That would capture all the integer zeroes
that appear directly in the pexe.

Optionally, capture the lookups of 0.0, but that is slightly tricky because you
want to match +0.0 and not -0.0.

https://codereview.chromium.org/1775253003/diff/20001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1775253003/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:471: RuntimeHelperFuncKind HelperName = H_Num;
Nit: it's more of a HelperID than a HelperName.

[Karl, 2016-03-16 22:50:52]

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

https://codereview.chromium.org/1775253003/diff/20001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:265: #define X(Ty)                                    
                             \
On 2016/03/09 23:26:56, John wrote:
> This looks dangerous in the context of the browser translator - when Ctx is
> deleted the array will point to bad data. I think this array should be a
> non-static member in global context.

Done.

https://codereview.chromium.org/1775253003/diff/20001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:266: ConstZeroForType[IceType_##Ty] =
Ctx->getConstantZero(IceType_##Ty);
On 2016/03/09 23:26:56, John wrote:
> It would be better if this used ICETYPE_TABLE... You could add a field to
those
> that is set to 1 if the type should have a constant zero

Since I moved the caching within method getConstantZero() within the global
context, this initialization is no longer necessary and has been removed.

https://codereview.chromium.org/1775253003/diff/20001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:270: // Define runtime helper functions.
Moved initialization of helper function names to global context as well.

https://codereview.chromium.org/1775253003/diff/20001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:307: Constant
*TargetLowering::ConstZeroForType[IceType_NUM] = {
Removed these initializations and moved to the constructor of the global
context.

https://codereview.chromium.org/1775253003/diff/20001/src/IceTargetLowering.def
File src/IceTargetLowering.def (right):

https://codereview.chromium.org/1775253003/diff/20001/src/IceTargetLowering.d...
src/IceTargetLowering.def:34: X(bitcast_16xi1_i16, "__Sz_bitcast_16xi1_i16")    
                          \
On 2016/03/10 00:24:46, stichnot wrote:
> Could you reformat this into aligned columns?

Done.

https://codereview.chromium.org/1775253003/diff/20001/src/IceTargetLowering.d...
src/IceTargetLowering.def:72: 
On 2016/03/10 00:24:46, stichnot wrote:
> remove one blank line

Done.

https://codereview.chromium.org/1775253003/diff/20001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/1775253003/diff/20001/src/IceTargetLowering.h...
src/IceTargetLowering.h:465: // Runtime helper function names
Also moved this enum to global context, so that we can move
getRuntimeHelperFunc() to global context.

https://codereview.chromium.org/1775253003/diff/20001/src/IceTargetLowering.h...
src/IceTargetLowering.h:580: /// Returns constant zero of Ty.
On 2016/03/10 00:24:46, stichnot wrote:
> You should probably also try to capture the calls to
> Ctx->getConstantInt{1,8,16,32,64}(0).  That would capture all the integer
zeroes
> that appear directly in the pexe.
> 
> Optionally, capture the lookups of 0.0, but that is slightly tricky because
you
> want to match +0.0 and not -0.0.

The plan was to capture more constants. This CL was to set up the framework for
extending to other constants. Did add additional code to capture cached values
for the different called paths for constants.

Also added "i1 true" to the list of cached constants.

https://codereview.chromium.org/1775253003/diff/20001/src/IceTargetLowering.h...
src/IceTargetLowering.h:589: Constant
*getRuntimeHelperFunc(RuntimeHelperFuncKind Kind) const {
Moved this to global context, and modified body (here) to simply call the
version in the global context.

https://codereview.chromium.org/1775253003/diff/20001/src/IceTargetLowering.h...
src/IceTargetLowering.h:600: static Constant *ConstZeroForType[IceType_NUM];
On 2016/03/09 23:26:56, John wrote:
> What do you think of making this array a non-static member of global context?
> Constants are tied to global context anyways.

Moved to global context, and used as cache inside method getConstantZero().

https://codereview.chromium.org/1775253003/diff/20001/src/IceTargetLowering.h...
src/IceTargetLowering.h:602: static Constant *RuntimeHelperFunc[H_Num];
Also moved this array to global context (as non-static).

https://codereview.chromium.org/1775253003/diff/20001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1775253003/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:471: RuntimeHelperFuncKind HelperName = H_Num;
On 2016/03/10 00:24:46, stichnot wrote:
> Nit: it's more of a HelperID than a HelperName.

Changed to HelperID.

[Jim Stichnoth, 2016-03-17 00:14:52]

https://codereview.chromium.org/1775253003/diff/60001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1775253003/diff/60001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:302: {
Is this set of braces necessary?

https://codereview.chromium.org/1775253003/diff/60001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:673: Constant
*GlobalContext::installConstantInt1(int8_t ConstantInt1) {
Bikeshed time.  I kinda don't like the "install" prefix, because it implies that
each call is actively installing (a new copy of) the constant somewhere.

(One could make a similar, perhaps weaker, argument about the "get" prefix
implying a read-only operation, but there's a lot of legacy and familiarity with
that prefix, so...)

What do you think about something like getConstantInt1Internal() ?

https://codereview.chromium.org/1775253003/diff/60001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:731: break;
You don't need all these "break" statements after a return.

https://codereview.chromium.org/1775253003/diff/60001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:751: return nullptr;
I kind of like the previous behavior where it gives an error for an
inappropriate type.

What do you think of adding a second parameter to this method, to enable hard
fail on invalid type?

Constant *GlobalContext::installConstantZero(Type Ty, bool AllowInvalidType =
false) {
  ...
  default:
    if (AllowInvalidType)
      return nullptr;
    llvm::report_fatal_error("Unknown type");
}

https://codereview.chromium.org/1775253003/diff/60001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/1775253003/diff/60001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:46: // Runtime helper function ID's
IDs

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

https://codereview.chromium.org/1775253003/diff/60001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:703: Operand *CallTarget =
getRuntimeHelperFunc(FuncID);
Keep this as Constant* ?

https://codereview.chromium.org/1775253003/diff/60001/src/IceTargetLowering.def
File src/IceTargetLowering.def (right):

https://codereview.chromium.org/1775253003/diff/60001/src/IceTargetLowering.d...
src/IceTargetLowering.def:58: 
remove one of these blank lines

https://codereview.chromium.org/1775253003/diff/60001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1775253003/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:578: getRuntimeHelperFunc(DestTy == IceType_f32 ?
H_frem_f32 : H_frem_f64);
It seems that TargetLowering::getRuntimeHelperFunc() just proxies to
GlobalContext::getRuntimeHelperFunc().  Can you get rid of the middleman and
directly call the GlobalContext version in all cases?  And remove the proxy
function from TargetLowering?

I assume this is just an artifact of the original implementation approach.

[John, 2016-03-17 14:31:33]

lgtm

https://codereview.chromium.org/1775253003/diff/60001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1775253003/diff/60001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:751: return nullptr;
On 2016/03/17 00:14:52, stichnot wrote:
> I kind of like the previous behavior where it gives an error for an
> inappropriate type.
> 
> What do you think of adding a second parameter to this method, to enable hard
> fail on invalid type?
> 
> Constant *GlobalContext::installConstantZero(Type Ty, bool AllowInvalidType =
> false) {
>   ...
>   default:
>     if (AllowInvalidType)
>       return nullptr;
>     llvm::report_fatal_error("Unknown type");
> }

I also like the previous behavior better, but I am not so sure about the extra
parameter. How about doing something like

Constant *GlobalContext::installConstantZero(Type Ty) {
  // current implementation.
}

Constant *GlobalContext::getConstantZero(Type Ty){
  auto *Zero = installConstantZero(Ty);
  if (Zero == nullptr) {
    report_fatal_error();
  }
  return Zero;
}

?

https://codereview.chromium.org/1775253003/diff/60001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/1775253003/diff/60001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:47: enum RuntimeHelperFuncID {
**I think** this should be an inner type (of GlobalContext.)

https://codereview.chromium.org/1775253003/diff/60001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:209: return installConstantInt1(ConstantInt1);
maybe assert here -- anything other than {0, 1} should be an error, right?

(if you add the assertion, please #include <cassert>)

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

https://codereview.chromium.org/1775253003/diff/60001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:703: Operand *CallTarget =
getRuntimeHelperFunc(FuncID);
On 2016/03/17 00:14:52, stichnot wrote:
> Keep this as Constant* ?

<joking message='did anyway say "auto *"? :)' />

https://codereview.chromium.org/1775253003/diff/60001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1775253003/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:578: getRuntimeHelperFunc(DestTy == IceType_f32 ?
H_frem_f32 : H_frem_f64);
On 2016/03/17 00:14:52, stichnot wrote:
> It seems that TargetLowering::getRuntimeHelperFunc() just proxies to
> GlobalContext::getRuntimeHelperFunc().  Can you get rid of the middleman and
> directly call the GlobalContext version in all cases?  And remove the proxy
> function from TargetLowering?
> 
> I assume this is just an artifact of the original implementation approach.

to avoid tons of changes, maybe just add a todo to remove
TargetLowering::getRuntimeHelperFunc()? I don't mind removing it later.

[Karl, 2016-03-17 16:40:53]

https://codereview.chromium.org/1775253003/diff/60001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1775253003/diff/60001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:302: {
On 2016/03/17 00:14:52, stichnot wrote:
> Is this set of braces necessary?

Removed.

https://codereview.chromium.org/1775253003/diff/60001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:673: Constant
*GlobalContext::installConstantInt1(int8_t ConstantInt1) {
On 2016/03/17 00:14:52, stichnot wrote:
> Bikeshed time.  I kinda don't like the "install" prefix, because it implies
that
> each call is actively installing (a new copy of) the constant somewhere.
> 
> (One could make a similar, perhaps weaker, argument about the "get" prefix
> implying a read-only operation, but there's a lot of legacy and familiarity
with
> that prefix, so...)
> 
> What do you think about something like getConstantInt1Internal() ?

Done.

https://codereview.chromium.org/1775253003/diff/60001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:731: break;
On 2016/03/17 00:14:52, stichnot wrote:
> You don't need all these "break" statements after a return.

Done.

https://codereview.chromium.org/1775253003/diff/60001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:751: return nullptr;
On 2016/03/17 14:31:33, John wrote:
> On 2016/03/17 00:14:52, stichnot wrote:
> > I kind of like the previous behavior where it gives an error for an
> > inappropriate type.
> > 
> > What do you think of adding a second parameter to this method, to enable
hard
> > fail on invalid type?
> > 
> > Constant *GlobalContext::installConstantZero(Type Ty, bool AllowInvalidType
=
> > false) {
> >   ...
> >   default:
> >     if (AllowInvalidType)
> >       return nullptr;
> >     llvm::report_fatal_error("Unknown type");
> > }
> 
> I also like the previous behavior better, but I am not so sure about the extra
> parameter. How about doing something like
> 
> Constant *GlobalContext::installConstantZero(Type Ty) {
>   // current implementation.
> }
> 
> Constant *GlobalContext::getConstantZero(Type Ty){
>   auto *Zero = installConstantZero(Ty);
>   if (Zero == nullptr) {
>     report_fatal_error();
>   }
>   return Zero;
> }
> 
> ?

Followed John's advice.

https://codereview.chromium.org/1775253003/diff/60001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/1775253003/diff/60001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:46: // Runtime helper function ID's
On 2016/03/17 00:14:52, stichnot wrote:
> IDs

Done.

https://codereview.chromium.org/1775253003/diff/60001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:47: enum RuntimeHelperFuncID {
On 2016/03/17 14:31:33, John wrote:
> **I think** this should be an inner type (of GlobalContext.)

I agree that this enum should be put in an enclosing scope, but I don't think it
should be GlobalContext. Adding namespace "RuntimeHelper" to namespace.

https://codereview.chromium.org/1775253003/diff/60001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:209: return installConstantInt1(ConstantInt1);
On 2016/03/17 14:31:33, John wrote:
> maybe assert here -- anything other than {0, 1} should be an error, right?
> 
> (if you add the assertion, please #include <cassert>)

Done.

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

https://codereview.chromium.org/1775253003/diff/60001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:703: Operand *CallTarget =
getRuntimeHelperFunc(FuncID);
On 2016/03/17 14:31:33, John wrote:
> On 2016/03/17 00:14:52, stichnot wrote:
> > Keep this as Constant* ?
> 
> <joking message='did anyway say "auto *"? :)' />

Changed to "Constant *".

https://codereview.chromium.org/1775253003/diff/60001/src/IceTargetLowering.def
File src/IceTargetLowering.def (right):

https://codereview.chromium.org/1775253003/diff/60001/src/IceTargetLowering.d...
src/IceTargetLowering.def:58: 
On 2016/03/17 00:14:52, stichnot wrote:
> remove one of these blank lines

Done.

https://codereview.chromium.org/1775253003/diff/60001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1775253003/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:578: getRuntimeHelperFunc(DestTy == IceType_f32 ?
H_frem_f32 : H_frem_f64);
On 2016/03/17 14:31:33, John wrote:
> On 2016/03/17 00:14:52, stichnot wrote:
> > It seems that TargetLowering::getRuntimeHelperFunc() just proxies to
> > GlobalContext::getRuntimeHelperFunc().  Can you get rid of the middleman and
> > directly call the GlobalContext version in all cases?  And remove the proxy
> > function from TargetLowering?
> > 
> > I assume this is just an artifact of the original implementation approach.
> 
> to avoid tons of changes, maybe just add a todo to remove
> TargetLowering::getRuntimeHelperFunc()? I don't mind removing it later.

See we scoped the enum RuntimeFunctionFuncID, we need to update the files
anyway. Removing.

[John, 2016-03-17 16:57:17]

https://codereview.chromium.org/1775253003/diff/60001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/1775253003/diff/60001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:47: enum RuntimeHelperFuncID {
On 2016/03/17 16:40:53, Karl wrote:
> On 2016/03/17 14:31:33, John wrote:
> > **I think** this should be an inner type (of GlobalContext.)
> 
> I agree that this enum should be put in an enclosing scope, but I don't think
it
> should be GlobalContext. Adding namespace "RuntimeHelper" to namespace.

Please, don't add a namespace. Use a class enum instead

[Karl, 2016-03-17 18:54:56]

https://codereview.chromium.org/1775253003/diff/60001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/1775253003/diff/60001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:47: enum RuntimeHelperFuncID {
On 2016/03/17 16:57:17, John wrote:
> On 2016/03/17 16:40:53, Karl wrote:
> > On 2016/03/17 14:31:33, John wrote:
> > > **I think** this should be an inner type (of GlobalContext.)
> > 
> > I agree that this enum should be put in an enclosing scope, but I don't
think
> it
> > should be GlobalContext. Adding namespace "RuntimeHelper" to namespace.
> 
> Please, don't add a namespace. Use a class enum instead

Done.

[Jim Stichnoth, 2016-03-17 20:12:23]

lgtm

https://codereview.chromium.org/1775253003/diff/100001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/1775253003/diff/100001/src/IceGlobalContext.h...
src/IceGlobalContext.h:267: Constant *Result =
RuntimeHelperFunc[static_cast<size_t>(FuncID)];
Can you can use the enum value directly without the size_t cast?  Or is this
something different about enum classes?

(here and below)

[John, 2016-03-17 20:22:57]

lgtm

https://codereview.chromium.org/1775253003/diff/100001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/1775253003/diff/100001/src/IceGlobalContext.h...
src/IceGlobalContext.h:267: Constant *Result =
RuntimeHelperFunc[static_cast<size_t>(FuncID)];
On 2016/03/17 20:12:22, stichnot wrote:
> Can you can use the enum value directly without the size_t cast?  Or is this
> something different about enum classes?
> 
> (here and below)

yup, he needs the cast -- class enums are just like that, not implicitly
convertible to/from integer types.

[Karl, 2016-03-17 20:30:16]

Description was changed from

==========
Cache common constants before lowering.

This code moves some constants used by the target lowering, so that they
are defined (and cached) during static initialization of the target
lowering, rather than looking up everytime they are used.

This CL does this for the constant zero, and predefined helper functions.

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

to

==========
Cache common constants before lowering.

This code moves some constants used by the target lowering, so that they
are defined (and cached) during static initialization of the target
lowering, rather than looking up everytime they are used.

This CL does this for the constant zero, and predefined helper functions.

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

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

[Karl, 2016-03-17 20:30:18]

Committed patchset #6 (id:100001) manually as
20070e84c21b9aa15a83e53b5d2ffa692c4e081a (presubmit successful).