Various improvements related to legalization code.

Various improvements related to legalization code.

1) In makeHelperCall(), function pointers that are created should have
type IceType_i32, not the functions' own return type.

2) In legalize(), change the name of WillHaveRegister to
MustHaveRegister. Add a comment to clarify the condition being computed.

3) In legalize(), add an assert to make sure that vector "constants"
don't get legalized (other than undef). There should be no constants of
vector type.

4) In copyToReg(), replace an unnecessary use of Src->getType().

BUG=none
R=stichnot@chromium.org

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

[Jim Stichnoth, 2014-07-14 19:44:17]

LGTM with a couple minor comments.

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

https://codereview.chromium.org/385133006/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:2675: bool AlreadyInRegister =
To me, WillHaveRegister captures the situation where no specific physical
register has been assigned/precolored, but it has been assigned infinite weight
so it is guaranteed to have a physical register in the future.

AlreadyInRegister only captures the precolored variable situation, IMO.

I would either find a more perfect name :) , or perhaps better yet just add this
clarification to the comment.

https://codereview.chromium.org/385133006/diff/1/src/IceTargetLoweringX8632.h
File src/IceTargetLoweringX8632.h (right):

https://codereview.chromium.org/385133006/diff/1/src/IceTargetLoweringX8632.h...
src/IceTargetLoweringX8632.h:134: Constant *CallTarget =
This makes me a little nervous, based on previous attempts to wrangle the type
system into line, but if all the tests pass, it must be OK. :)

[wala, 2014-07-14 20:40:03]

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

https://codereview.chromium.org/385133006/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:2675: bool AlreadyInRegister =
On 2014/07/14 19:44:17, stichnot wrote:
> To me, WillHaveRegister captures the situation where no specific physical
> register has been assigned/precolored, but it has been assigned infinite
weight
> so it is guaranteed to have a physical register in the future.
> 
> AlreadyInRegister only captures the precolored variable situation, IMO.
> 
> I would either find a more perfect name :) , or perhaps better yet just add
this
> clarification to the comment.

How about MustHaveRegister?

For me, WillHaveRegister suggests the meaning that the variable will be assigned
to a register (and it's not necessarily precolored or infinite weight), which of
course can't be computed at this stage.

[Jim Stichnoth, 2014-07-14 20:51:52]

On 2014/07/14 20:40:03, wala wrote:
>
https://codereview.chromium.org/385133006/diff/1/src/IceTargetLoweringX8632.cpp
> File src/IceTargetLoweringX8632.cpp (right):
> 
>
https://codereview.chromium.org/385133006/diff/1/src/IceTargetLoweringX8632.c...
> src/IceTargetLoweringX8632.cpp:2675: bool AlreadyInRegister =
> On 2014/07/14 19:44:17, stichnot wrote:
> > To me, WillHaveRegister captures the situation where no specific physical
> > register has been assigned/precolored, but it has been assigned infinite
> weight
> > so it is guaranteed to have a physical register in the future.
> > 
> > AlreadyInRegister only captures the precolored variable situation, IMO.
> > 
> > I would either find a more perfect name :) , or perhaps better yet just add
> this
> > clarification to the comment.
> 
> How about MustHaveRegister?
> 
> For me, WillHaveRegister suggests the meaning that the variable will be
assigned
> to a register (and it's not necessarily precolored or infinite weight), which
of
> course can't be computed at this stage.

Good point.  Another possibility might be something like ForcedRegister.  I
don't feel strongly about the specific choice at this point, but all this
discussion suggests that a short comment in the code is in order. :)

[wala, 2014-07-15 00:37:40]

Committed patchset #2 manually as rad8f726 (presubmit successful).