[Subzero][MIPS32] Account for variable alloca alignment bytes in addProlog

[Subzero][MIPS32] Account for variable alloca alignment bytes in addProlog

R=stichnot@chromium.org

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

[sagar.thakur, 2016-10-17 12:48:18]

sagar.thakur@imgtec.com changed reviewers:
+ eholk@chromium.org, jaydeep.patil@imgtec.com, jpp@chromium.org,
kschimpf@google.com, stichnot@chromium.org

[Jim Stichnoth, 2016-10-17 14:20:46]

https://codereview.chromium.org/2425673002/diff/20001/src/IceInstMIPS32.h
File src/IceInstMIPS32.h (right):

https://codereview.chromium.org/2425673002/diff/20001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:1057: uint32_t getImmidiateValue() { return Imm; }
Fix spelling, plus "const".
uint32_t getImmediateValue() const { return Imm; }

https://codereview.chromium.org/2425673002/diff/20001/src/IceTargetLoweringMI...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2425673002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1381: if (!VariableAllocaUsed)
Maybe this could be simplified/refactored slightly?

if (VariableAllocaUsed)
  SpillAreaSizeBytes += VariableAllocaAlignBytes;
else
  SpillAreaSizeBytes += MaxOutArgsSizeBytes;
SpillAreaSizeBytes = applyStackAlignment(SpillAreaSizeBytes);

or even combined into one assignment:

SpillAreaSizeBytes = applyStackAlignment(SpillAreaSizeBytes +
(VariableAllocaUsed ? VariableAllocaAlignBytes : MaxOutArgsSizeBytes));

https://codereview.chromium.org/2425673002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1794: Variable
*TargetMIPS32::PostLoweringLegalizer::legalizeImmidiate(int32_t Imm) {
legalizeImmediate

https://codereview.chromium.org/2425673002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1798: } else {
http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

https://codereview.chromium.org/2425673002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1799: uint32_t UpperBits = (Imm >> 16) & 0xFFFF;
const ?

https://codereview.chromium.org/2425673002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1877: int32_t(AddiuInstr->getImmidiateValue())))
{
Use reinterpret_cast<> instead of int32_t() ?

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

https://codereview.chromium.org/2425673002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.h:737: /// Legalizes Immidiate if larger value
overflows range of 16 bits
Immediate

[sagar.thakur, 2016-10-18 07:02:21]

https://codereview.chromium.org/2425673002/diff/20001/src/IceInstMIPS32.h
File src/IceInstMIPS32.h (right):

https://codereview.chromium.org/2425673002/diff/20001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:1057: uint32_t getImmidiateValue() { return Imm; }
On 2016/10/17 14:20:45, Jim Stichnoth wrote:
> Fix spelling, plus "const".
> uint32_t getImmediateValue() const { return Imm; }

Done.

https://codereview.chromium.org/2425673002/diff/20001/src/IceTargetLoweringMI...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2425673002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1381: if (!VariableAllocaUsed)
On 2016/10/17 14:20:46, Jim Stichnoth wrote:
> Maybe this could be simplified/refactored slightly?
> 
> if (VariableAllocaUsed)
>   SpillAreaSizeBytes += VariableAllocaAlignBytes;
> else
>   SpillAreaSizeBytes += MaxOutArgsSizeBytes;
> SpillAreaSizeBytes = applyStackAlignment(SpillAreaSizeBytes);
> 
> or even combined into one assignment:
> 
> SpillAreaSizeBytes = applyStackAlignment(SpillAreaSizeBytes +
> (VariableAllocaUsed ? VariableAllocaAlignBytes : MaxOutArgsSizeBytes));

Done.

https://codereview.chromium.org/2425673002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1794: Variable
*TargetMIPS32::PostLoweringLegalizer::legalizeImmidiate(int32_t Imm) {
On 2016/10/17 14:20:46, Jim Stichnoth wrote:
> legalizeImmediate

Done.

https://codereview.chromium.org/2425673002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1798: } else {
On 2016/10/17 14:20:45, Jim Stichnoth wrote:
> http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

Done.

https://codereview.chromium.org/2425673002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1799: uint32_t UpperBits = (Imm >> 16) & 0xFFFF;
On 2016/10/17 14:20:46, Jim Stichnoth wrote:
> const ?

Done.

https://codereview.chromium.org/2425673002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1877: int32_t(AddiuInstr->getImmidiateValue())))
{
On 2016/10/17 14:20:46, Jim Stichnoth wrote:
> Use reinterpret_cast<> instead of int32_t() ?

Done. Used static cast because reinterpret cast does not allow casting from
unsigned to signed.

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

https://codereview.chromium.org/2425673002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.h:737: /// Legalizes Immidiate if larger value
overflows range of 16 bits
On 2016/10/17 14:20:46, Jim Stichnoth wrote:
> Immediate

Done.

[Jim Stichnoth, 2016-10-18 14:31:48]

lgtm

[Jim Stichnoth, 2016-10-18 14:32:24]

Description was changed from

==========
[Subzero][MIPS32] Account for variable alloca alignment bytes in addProlog
==========

to

==========
[Subzero][MIPS32] Account for variable alloca alignment bytes in addProlog

R=stichnot@chromium.org

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

[Jim Stichnoth, 2016-10-18 14:32:25]

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