Fix a bug that would cause subzero to fail when --threads=0.

Fix a bug that would cause subzero to fail when --threads=0.

Creates a single TargetDataLowering.

BUG= None
R=stichnot@chromium.org

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

[John, 2015-06-16 15:54:03]

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

[Karl, 2015-06-16 16:13:45]

kschimpf@google.com changed reviewers:
+ kschimpf@google.com
- kschimpf@chromium.org

[Jim Stichnoth, 2015-06-16 17:03:25]

https://codereview.chromium.org/1179313004/diff/70001/src/IceELFObjectWriter.h
File src/IceELFObjectWriter.h (right):

https://codereview.chromium.org/1179313004/diff/70001/src/IceELFObjectWriter....
src/IceELFObjectWriter.h:34: // SectionSuffix is unique)
can you indent this by 4 characters?

https://codereview.chromium.org/1179313004/diff/70001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/1179313004/diff/70001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:447: std::unique_ptr<TargetDataLowering> DataLowering;
Non-const fields in the GlobalContext object generally need to be accessed while
holding a lock.  (The intention is that anything that might need a lock is part
of GlobalContext.)

https://codereview.chromium.org/1179313004/diff/70001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1179313004/diff/70001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:18: #include <sstream>
http://llvm.org/docs/CodingStandards.html#use-raw-ostream

See existing uses of llvm::raw_string_ostream .

https://codereview.chromium.org/1179313004/diff/70001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:453: IceString dataSectionSuffix(const IceString
&SectionSuffix,
I realize that -fdata-sections hasn't been implemented yet, but if it does get
implemented, this suffix mechanism will make Subzero's data section names
incompatible with llc's data section names, which will affect bisection
debugging.

Would it make sense to print a warning if the user tries to use -fdata-sections
and -enable-block-profile at the same time?

[John, 2015-06-16 17:30:55]

https://codereview.chromium.org/1179313004/diff/70001/src/IceELFObjectWriter.h
File src/IceELFObjectWriter.h (right):

https://codereview.chromium.org/1179313004/diff/70001/src/IceELFObjectWriter....
src/IceELFObjectWriter.h:34: // SectionSuffix is unique)
On 2015/06/16 17:03:24, stichnot wrote:
> can you indent this by 4 characters?

Done.

https://codereview.chromium.org/1179313004/diff/70001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/1179313004/diff/70001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:447: std::unique_ptr<TargetDataLowering> DataLowering;
On 2015/06/16 17:03:24, stichnot wrote:
> Non-const fields in the GlobalContext object generally need to be accessed
while
> holding a lock.  (The intention is that anything that might need a lock is
part
> of GlobalContext.)

The DataLowering is only ever used in
(1) emitItems; or
(2) IceCompiler::run when emitItems is done emitting items.

In both those contexts the use is sequential (in either IceCompilerServer and
IceBrowserCompileServer the context is created in a function and a compile
thread is started.) I added a comment, but happy to add a lock.

https://codereview.chromium.org/1179313004/diff/70001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1179313004/diff/70001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:18: #include <sstream>
On 2015/06/16 17:03:25, stichnot wrote:
> http://llvm.org/docs/CodingStandards.html#use-raw-ostream
> 
> See existing uses of llvm::raw_string_ostream .

no longer relevant here, but Done for IceELFObjectWriter.

https://codereview.chromium.org/1179313004/diff/70001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:453: IceString dataSectionSuffix(const IceString
&SectionSuffix,
On 2015/06/16 17:03:24, stichnot wrote:
> I realize that -fdata-sections hasn't been implemented yet, but if it does get
> implemented, this suffix mechanism will make Subzero's data section names
> incompatible with llc's data section names, which will affect bisection
> debugging.
> 
> Would it make sense to print a warning if the user tries to use
-fdata-sections
> and -enable-block-profile at the same time?

Well, with -fdata-sections we don't need the SectionSuffix (each profile
variable will live in its on section.) I modified the logic here so that, with
-fdata-sections SectionSuffix is simply ignored.

[jvoung (off chromium), 2015-06-16 18:23:10]

Made a quick scan over the elf object writer

https://codereview.chromium.org/1179313004/diff/90001/src/IceCompiler.cpp
File src/IceCompiler.cpp (right):

https://codereview.chromium.org/1179313004/diff/90001/src/IceCompiler.cpp#new...
src/IceCompiler.cpp:155: Ctx.getObjectWriter()->writeNonUserSections();
Should this be under that else { } branch also, if the else is to avoid doing
extra work when there is a Translator error?

https://codereview.chromium.org/1179313004/diff/90001/src/IceELFObjectWriter.cpp
File src/IceELFObjectWriter.cpp (right):

https://codereview.chromium.org/1179313004/diff/90001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:14: #include <sstream>
remove include?

https://codereview.chromium.org/1179313004/diff/90001/src/IceELFObjectWriter.h
File src/IceELFObjectWriter.h (right):

https://codereview.chromium.org/1179313004/diff/90001/src/IceELFObjectWriter....
src/IceELFObjectWriter.h:40: // The requirement for writeDataSection to be
invoked only once can
Maybe just remove this "invoked only once" note.

https://codereview.chromium.org/1179313004/diff/90001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1179313004/diff/90001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:451: // dataSectionSuffix decides wether to use
SectionSuffix or MangledVarName as
wether -> whether

https://codereview.chromium.org/1179313004/diff/90001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:459: // avoid constructing the ostringstream below.
"ostringstream" ?

[John, 2015-06-16 19:58:02]

https://codereview.chromium.org/1179313004/diff/90001/src/IceCompiler.cpp
File src/IceCompiler.cpp (right):

https://codereview.chromium.org/1179313004/diff/90001/src/IceCompiler.cpp#new...
src/IceCompiler.cpp:155: Ctx.getObjectWriter()->writeNonUserSections();
On 2015/06/16 18:23:10, jvoung wrote:
> Should this be under that else { } branch also, if the else is to avoid doing
> extra work when there is a Translator error?

No strong opinions here, so done. Happy to revert.

https://codereview.chromium.org/1179313004/diff/90001/src/IceELFObjectWriter.cpp
File src/IceELFObjectWriter.cpp (right):

https://codereview.chromium.org/1179313004/diff/90001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:14: #include <sstream>
On 2015/06/16 18:23:10, jvoung wrote:
> remove include?

Done.

https://codereview.chromium.org/1179313004/diff/90001/src/IceELFObjectWriter.h
File src/IceELFObjectWriter.h (right):

https://codereview.chromium.org/1179313004/diff/90001/src/IceELFObjectWriter....
src/IceELFObjectWriter.h:40: // The requirement for writeDataSection to be
invoked only once can
On 2015/06/16 18:23:10, jvoung wrote:
> Maybe just remove this "invoked only once" note.

Done.

https://codereview.chromium.org/1179313004/diff/90001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1179313004/diff/90001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:451: // dataSectionSuffix decides wether to use
SectionSuffix or MangledVarName as
On 2015/06/16 18:23:10, jvoung wrote:
> wether -> whether

Done.

https://codereview.chromium.org/1179313004/diff/90001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:459: // avoid constructing the ostringstream below.
On 2015/06/16 18:23:10, jvoung wrote:
> "ostringstream" ?

Done.

[Jim Stichnoth, 2015-06-17 16:04:57]

https://codereview.chromium.org/1179313004/diff/70001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/1179313004/diff/70001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:447: std::unique_ptr<TargetDataLowering> DataLowering;
On 2015/06/16 17:30:55, John wrote:
> On 2015/06/16 17:03:24, stichnot wrote:
> > Non-const fields in the GlobalContext object generally need to be accessed
> while
> > holding a lock.  (The intention is that anything that might need a lock is
> part
> > of GlobalContext.)
> 
> The DataLowering is only ever used in
> (1) emitItems; or
> (2) IceCompiler::run when emitItems is done emitting items.
> 
> In both those contexts the use is sequential (in either IceCompilerServer and
> IceBrowserCompileServer the context is created in a function and a compile
> thread is started.) I added a comment, but happy to add a lock.

I do think the fields should use a lock, to protect against future misuse. 
Maybe bundle those 3 fields into a class and wrap that object in LockedPtr<> ?

https://codereview.chromium.org/1179313004/diff/110001/src/IceELFObjectWriter...
File src/IceELFObjectWriter.cpp (right):

https://codereview.chromium.org/1179313004/diff/110001/src/IceELFObjectWriter...
src/IceELFObjectWriter.cpp:308: IceString MangledName;
Can you just
  return Base + "." + Suffix;
?
and then remove the raw_ostream include?

https://codereview.chromium.org/1179313004/diff/110001/src/IceELFObjectWriter.h
File src/IceELFObjectWriter.h (right):

https://codereview.chromium.org/1179313004/diff/110001/src/IceELFObjectWriter...
src/IceELFObjectWriter.h:32: // (1) writeInitialELFHeader (invoke once)
Hi, my name is Jim, and I suffer from a problem.

(Hi Jim!)

Maybe make this comment more beautiful thusly?

// (1) writeInitialELFHeader (invoke once)
// (2) writeDataSection      (may be invoked multiple times, as long as
//                            SectionSuffix is unique)
// (3) writeFunctionCode     (must invoke once per function)
// (4) writeConstantPool     (must invoke once per pooled primitive type)
// (5) setUndefinedSyms      (invoke once)

https://codereview.chromium.org/1179313004/diff/110001/src/IceTargetLoweringX...
File src/IceTargetLoweringX8632.h (right):

https://codereview.chromium.org/1179313004/diff/110001/src/IceTargetLoweringX...
src/IceTargetLoweringX8632.h:592: void lowerConstants(const IceString
&SectionSuffix) override;
I don't see the SectionSuffix argument actually used in any of the
implementations.  Is that a mistake?  Is it there for future reference?  Can it
be removed?

[John, 2015-06-17 18:18:35]

https://codereview.chromium.org/1179313004/diff/110001/src/IceELFObjectWriter...
File src/IceELFObjectWriter.cpp (right):

https://codereview.chromium.org/1179313004/diff/110001/src/IceELFObjectWriter...
src/IceELFObjectWriter.cpp:308: IceString MangledName;
On 2015/06/17 16:04:56, stichnot wrote:
> Can you just
>   return Base + "." + Suffix;
> ?
> and then remove the raw_ostream include?

Not that it matters, but I have a hunch using the ostream is more efficient.
Done, though.

https://codereview.chromium.org/1179313004/diff/110001/src/IceELFObjectWriter.h
File src/IceELFObjectWriter.h (right):

https://codereview.chromium.org/1179313004/diff/110001/src/IceELFObjectWriter...
src/IceELFObjectWriter.h:32: // (1) writeInitialELFHeader (invoke once)
On 2015/06/17 16:04:56, stichnot wrote:
> Hi, my name is Jim, and I suffer from a problem.
> 
> (Hi Jim!)
Hi Jim!

> 
> Maybe make this comment more beautiful thusly?
> 
> // (1) writeInitialELFHeader (invoke once)
> // (2) writeDataSection      (may be invoked multiple times, as long as
> //                            SectionSuffix is unique)
> // (3) writeFunctionCode     (must invoke once per function)
> // (4) writeConstantPool     (must invoke once per pooled primitive type)
> // (5) setUndefinedSyms      (invoke once)

Beauty is in the eye of the beholder -- I prefer the way the comment looks now,
but no strong opinions. :) Done.

https://codereview.chromium.org/1179313004/diff/110001/src/IceTargetLoweringX...
File src/IceTargetLoweringX8632.h (right):

https://codereview.chromium.org/1179313004/diff/110001/src/IceTargetLoweringX...
src/IceTargetLoweringX8632.h:592: void lowerConstants(const IceString
&SectionSuffix) override;
On 2015/06/17 16:04:56, stichnot wrote:
> I don't see the SectionSuffix argument actually used in any of the
> implementations.  Is that a mistake?  Is it there for future reference?  Can
it
> be removed?

This was just for future proofing -- and also to keep the interface somewhat
consistent. Done.

[Jim Stichnoth, 2015-06-17 20:13:57]

lgtm

[John, 2015-06-17 20:20:13]

Committed patchset #8 (id:130001) manually as
8b1a705165292328a3db53da273420989b46e443 (tree was closed).