Subzero. Refactors Switch Lowering.

Subzero. Refactors Switch Lowering.

BUG=
R=stichnot@chromium.org

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

[John, 2016-04-04 12:43:16]

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

[John, 2016-04-04 12:51:30]

my verify_spec.sh script tells me the output is unchanged.

[Jim Stichnoth, 2016-04-04 13:42:30]

LGTM.

I assume fixing the unconditional instantiation of global strings for jump table
labels, will be a separate CL.  If so, feel free to defer my comments until
later.

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

https://codereview.chromium.org/1860473002/diff/100001/src/IceGlobalContext.h...
src/IceGlobalContext.h:272: /// Adds JumpTable to the list of know jump tables,
for a posterior emission.
known

https://codereview.chromium.org/1860473002/diff/120001/src/IceELFObjectWriter...
File src/IceELFObjectWriter.cpp (right):

https://codereview.chromium.org/1860473002/diff/120001/src/IceELFObjectWriter...
src/IceELFObjectWriter.cpp:586: IsPIC ? ".data.rel.ro" : ".rodata",
JT.getSectionName());
It bothers me a tiny bit that we are instantiating the getSectionName() string.

I wonder if maybe we should only call MangleSectionName if -fdata-sections is
specified?

The same would apply to the x86 target emit routines.

https://codereview.chromium.org/1860473002/diff/120001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1860473002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:7291: Str << JumpTable->getName() << ":";
What do you think about refactoring this piece to look like the chunk below
under case FT_Iasm?

[John, 2016-04-04 14:31:05]

"I assume fixing the unconditional instantiation of global strings for jump
table
labels, will be a separate CL.  If so, feel free to defer my comments until
later."

What do you mean, exactly? (I am sleep deprived, and parsing English becomes a
lot harder the more tired I am... :P) I was hoping to land a single CL fixing
this.

https://codereview.chromium.org/1860473002/diff/120001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1860473002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:7291: Str << JumpTable->getName() << ":";
On 2016/04/04 13:42:30, stichnot wrote:
> What do you think about refactoring this piece to look like the chunk below
> under case FT_Iasm?

well, an even better question is, why do we need both of these?! and why doesn't
the code below invokes this?!

[Jim Stichnoth, 2016-04-04 14:38:37]

On 2016/04/04 14:31:05, John wrote:
> "I assume fixing the unconditional instantiation of global strings for jump
> table
> labels, will be a separate CL.  If so, feel free to defer my comments until
> later."
> 
> What do you mean, exactly? (I am sleep deprived, and parsing English becomes a
> lot harder the more tired I am... :P) I was hoping to land a single CL fixing
> this.
> 
>
https://codereview.chromium.org/1860473002/diff/120001/src/IceTargetLoweringX...
> File src/IceTargetLoweringX86BaseImpl.h (right):
> 
>
https://codereview.chromium.org/1860473002/diff/120001/src/IceTargetLoweringX...
> src/IceTargetLoweringX86BaseImpl.h:7291: Str << JumpTable->getName() << ":";
> On 2016/04/04 13:42:30, stichnot wrote:
> > What do you think about refactoring this piece to look like the chunk below
> > under case FT_Iasm?
> 
> well, an even better question is, why do we need both of these?! and why
doesn't
> the code below invokes this?!

If you make a MINIMAL build, and run "pnacl-sz ... -dump-strings", we don't want
to see any ".L12345_1" style strings in the output.

[John, 2016-04-05 12:22:43]

There's really no way to **not** name a jump table and still expect
deterministic output. not with the current implementation, at least.

The problem lies in IceTargetLoweringX86::lowerCaseCluster. That method created
a memory operand that references the jump table by name so, at that point, a
name must be created for the jump table. It does not matter what that name is,
but once it is picked, it can't be changed.

We could pre-allocate those strings during parsing, but our new, awesome
parallel parsing introduces the same problem.

This seems to be minor enough that this is better left alone -- otherwise, we're
complicating Subzero (which is already complicated enough) too much for very
little gain. Unless it can be proven that the global jump table name creation is
a problem, I suggest we treat it as not a problem and leave the current
implementation as is.

I am going ahead and landing this if make presubmit is happy.

https://codereview.chromium.org/1860473002/diff/120001/src/IceELFObjectWriter...
File src/IceELFObjectWriter.cpp (right):

https://codereview.chromium.org/1860473002/diff/120001/src/IceELFObjectWriter...
src/IceELFObjectWriter.cpp:586: IsPIC ? ".data.rel.ro" : ".rodata",
JT.getSectionName());
On 2016/04/04 13:42:30, stichnot wrote:
> It bothers me a tiny bit that we are instantiating the getSectionName()
string.
> 
> I wonder if maybe we should only call MangleSectionName if -fdata-sections is
> specified?
> 
> The same would apply to the x86 target emit routines.

This would change sz's output, so I'll leave as is.

This seems to be part of a larger "problem": we don't have a centralized
"section name factory," which would be nice to have so that we would have
control over how/when these strings are created.

https://codereview.chromium.org/1860473002/diff/120001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1860473002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:7291: Str << JumpTable->getName() << ":";
On 2016/04/04 13:42:30, stichnot wrote:
> What do you think about refactoring this piece to look like the chunk below
> under case FT_Iasm?

Done.

[John, 2016-04-05 13:30:24]

Description was changed from

==========
Subzero. Refactors Switch Lowering.

BUG=
==========

to

==========
Subzero. Refactors Switch Lowering.

BUG=
R=stichnot@chromium.org

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

[John, 2016-04-05 13:30:26]

Committed patchset #9 (id:160001) manually as
030772114216c1a57c749050bb58d07de8ceaa7c (presubmit successful).