Add phi instruction to Subzero bitcode reader.

Add phi instruction to Subzero bitcode reader.

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

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

[Karl, 2014-09-11 16:57:27]

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

[Jim Stichnoth, 2014-09-11 18:26:40]

lgtm, including the non-phi-related refactoring :)

https://codereview.chromium.org/568473002/diff/20001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/568473002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1622: unsigned NumValues = Values.size()/2;
Maybe use "Values.size() >> 1" to make it a little more clear that you're
discarding the odd bit?

Also, if you just move this expression into the InstPhi::create() call, you
won't even have to change "unsigned" to "Ice::SizeT". :)

https://codereview.chromium.org/568473002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1637: StrBuf << "Phi node expects type " << Ty << " but
found: " << Op;
Nit: maybe "operand" or "instruction" instead of "node"?

https://codereview.chromium.org/568473002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1641: Phi->addArgument(Op, getBasicBlock(Values[i+1]));
hmm, did clang-format really allow "i+1" without spaces? :)

[jvoung (off chromium), 2014-09-11 19:14:43]

LGTM too (tiny nit)

https://codereview.chromium.org/568473002/diff/20001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/568473002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1611: // PHI: [ty, val0,bb0, ...]
add a space between val0, bb0 ?

[Jim Stichnoth, 2014-09-11 19:31:29]

https://codereview.chromium.org/568473002/diff/20001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/568473002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1611: // PHI: [ty, val0,bb0, ...]
On 2014/09/11 19:14:43, jvoung wrote:
> add a space between val0, bb0 ?

I assumed that was intentional grouping, e.g.
  [ty, val0,bb0, val1,bb1, val2,bb2, ...]

[Karl, 2014-09-11 21:42:56]

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

[Karl, 2014-09-11 21:43:04]

https://codereview.chromium.org/568473002/diff/20001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/568473002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1611: // PHI: [ty, val0,bb0, ...]
On 2014/09/11 19:31:29, stichnot wrote:
> On 2014/09/11 19:14:43, jvoung wrote:
> > add a space between val0, bb0 ?
> 
> I assumed that was intentional grouping, e.g.
>   [ty, val0,bb0, val1,bb1, val2,bb2, ...]

Actually, the space probably be added. Clarifying ... by adding pair "valN, bbN
at end.

https://codereview.chromium.org/568473002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1622: unsigned NumValues = Values.size()/2;
On 2014/09/11 18:26:40, stichnot wrote:
> Maybe use "Values.size() >> 1" to make it a little more clear that you're
> discarding the odd bit?
> 
> Also, if you just move this expression into the InstPhi::create() call, you
> won't even have to change "unsigned" to "Ice::SizeT". :)

Done.

https://codereview.chromium.org/568473002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1637: StrBuf << "Phi node expects type " << Ty << " but
found: " << Op;
On 2014/09/11 18:26:40, stichnot wrote:
> Nit: maybe "operand" or "instruction" instead of "node"?

Done.

https://codereview.chromium.org/568473002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1641: Phi->addArgument(Op, getBasicBlock(Values[i+1]));
On 2014/09/11 18:26:40, stichnot wrote:
> hmm, did clang-format really allow "i+1" without spaces? :)

Done.