Subzero. Enables moar complex relocation offsets.

Subzero. Enables moar complex relocation offsets.

This CL allows ConstantRelocatables in to Subzero
have symbolic constants. A symbolic constant is an
assembly label whose value is not known during
lowering, but it is well defined during code
emission.

For example, the following code is now possible in Subzero:

foo:
  push $foo.bar
  jmp target
  nop
  nop
foo.bar:
  ...

BUG=
R=stichnot@chromium.org

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

[John, 2016-02-01 23:00:51]

Description was changed from

==========
Subzero. Enables moar complex relocation offsets.

TODO(jpp): DESCRIBE THE CL APPROPRIATELY.

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

to

==========
Subzero. Enables moar complex relocation offsets.

This CL allows ConstantRelocatables in to Subzero
have symbolic constants. A symbolic constant is an
assembly label whose value is not known during
lowering, but it is well defined during code
emission.

For example, the following code is now possible in Subzero:

foo:
  push $foo.bar
  jmp target
  nop
  nop
foo.bar:
  ...

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

[John, 2016-02-02 00:35:18]

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

[Jim Stichnoth, 2016-02-02 17:13:45]

https://codereview.chromium.org/1651163002/diff/60001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1651163002/diff/60001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:47: return hash<Ice::IceString>()(Key.Name);
Any reason not to add some aspect of OffsetExpr into the hash?

https://codereview.chromium.org/1651163002/diff/60001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/1651163002/diff/60001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:204: Constant *getConstantSym(std::vector<RelocOffset *>
Offset,
My radar goes off when I see the lack of const ref arg, but then I see some
std::move trickery along with {initializer} syntax, so presumably it needs to be
that way?

Same with emitString() in another file.

I'll take my answer offline. :)

https://codereview.chromium.org/1651163002/diff/60001/src/IceOperand.cpp
File src/IceOperand.cpp (right):

https://codereview.chromium.org/1651163002/diff/60001/src/IceOperand.cpp#newc...
src/IceOperand.cpp:16: #include <iostream>
System includes go at the end of the list.

https://codereview.chromium.org/1651163002/diff/60001/src/IceOperand.cpp#newc...
src/IceOperand.cpp:33: // (1) is trivial to implement, but (2) requires some
care.
I would s/implement/check/ .

https://codereview.chromium.org/1651163002/diff/60001/src/IceOperand.cpp#newc...
src/IceOperand.cpp:39: //    value) than B.OffsetExpr[i].
s/than/as/

https://codereview.chromium.org/1651163002/diff/60001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/1651163002/diff/60001/src/IceOperand.h#newcod...
src/IceOperand.h:303: std::vector<RelocOffset *> OffsetExpr;
const

https://codereview.chromium.org/1651163002/diff/60001/src/IceOperand.h#newcod...
src/IceOperand.h:332: void setEmitString(IceString Value) {
Generally, Constants should not have setters, since the objects are shared
across threads.  I think you can and should make this part of the ctor.

I'm not sure why SuppressMangling got an exception to this.

[John, 2016-02-02 19:36:39]

https://codereview.chromium.org/1651163002/diff/60001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1651163002/diff/60001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:47: return hash<Ice::IceString>()(Key.Name);
On 2016/02/02 17:13:44, stichnot wrote:
> Any reason not to add some aspect of OffsetExpr into the hash?

Done.

https://codereview.chromium.org/1651163002/diff/60001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/1651163002/diff/60001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:204: Constant *getConstantSym(std::vector<RelocOffset *>
Offset,
On 2016/02/02 17:13:44, stichnot wrote:
> My radar goes off when I see the lack of const ref arg, but then I see some
> std::move trickery along with {initializer} syntax, so presumably it needs to
be
> that way?
> 
> Same with emitString() in another file.
> 
> I'll take my answer offline. :)

Acknowledged.

https://codereview.chromium.org/1651163002/diff/60001/src/IceOperand.cpp
File src/IceOperand.cpp (right):

https://codereview.chromium.org/1651163002/diff/60001/src/IceOperand.cpp#newc...
src/IceOperand.cpp:16: #include <iostream>
On 2016/02/02 17:13:44, stichnot wrote:
> System includes go at the end of the list.

Done.

https://codereview.chromium.org/1651163002/diff/60001/src/IceOperand.cpp#newc...
src/IceOperand.cpp:33: // (1) is trivial to implement, but (2) requires some
care.
On 2016/02/02 17:13:44, stichnot wrote:
> I would s/implement/check/ .

Done.

https://codereview.chromium.org/1651163002/diff/60001/src/IceOperand.cpp#newc...
src/IceOperand.cpp:39: //    value) than B.OffsetExpr[i].
On 2016/02/02 17:13:44, stichnot wrote:
> s/than/as/

Done.

https://codereview.chromium.org/1651163002/diff/60001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/1651163002/diff/60001/src/IceOperand.h#newcod...
src/IceOperand.h:303: std::vector<RelocOffset *> OffsetExpr;
On 2016/02/02 17:13:44, stichnot wrote:
> const

Done.

https://codereview.chromium.org/1651163002/diff/60001/src/IceOperand.h#newcod...
src/IceOperand.h:332: void setEmitString(IceString Value) {
On 2016/02/02 17:13:44, stichnot wrote:
> Generally, Constants should not have setters, since the objects are shared
> across threads.  I think you can and should make this part of the ctor.
> 
> I'm not sure why SuppressMangling got an exception to this.

Done.

[Jim Stichnoth, 2016-02-02 20:57:49]

lgtm

https://codereview.chromium.org/1651163002/diff/80001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/1651163002/diff/80001/src/IceDefs.h#newcode313
src/IceDefs.h:313: using RelocOffsetArray = llvm::SmallVector<RelocOffset *, 4>;
You could use "class RelocOffset" here and remove the explicit forward
declaration above, if you prefer.

https://codereview.chromium.org/1651163002/diff/80001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1651163002/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:50: if (Key.OffsetExpr[0]->hasOffset()) {
Please assert(!Key.OffsetExpr.empty())
otherwise it looks like a bug.

https://codereview.chromium.org/1651163002/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:55: hash<std::size_t>()(Key.OffsetExpr.size());
remove this

https://codereview.chromium.org/1651163002/diff/80001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/1651163002/diff/80001/src/IceOperand.h#newcode29
src/IceOperand.h:29: #include <limits>
I don't think this is actually necessary, is it?

https://codereview.chromium.org/1651163002/diff/80001/src/IceOperand.h#newcod...
src/IceOperand.h:365: const IceString EmitString;        /// optional for
textual output
I would change "output" to "asm" or "emission"

[John, 2016-02-02 23:06:12]

Description was changed from

==========
Subzero. Enables moar complex relocation offsets.

This CL allows ConstantRelocatables in to Subzero
have symbolic constants. A symbolic constant is an
assembly label whose value is not known during
lowering, but it is well defined during code
emission.

For example, the following code is now possible in Subzero:

foo:
  push $foo.bar
  jmp target
  nop
  nop
foo.bar:
  ...

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

to

==========
Subzero. Enables moar complex relocation offsets.

This CL allows ConstantRelocatables in to Subzero
have symbolic constants. A symbolic constant is an
assembly label whose value is not known during
lowering, but it is well defined during code
emission.

For example, the following code is now possible in Subzero:

foo:
  push $foo.bar
  jmp target
  nop
  nop
foo.bar:
  ...

BUG=
R=stichnot@chromium.org

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

[John, 2016-02-02 23:06:13]

Committed patchset #7 (id:120001) manually as
27fddcc36ad3f0c7531e481f9b249bd36ef83057 (presubmit successful).

[John, 2016-02-03 15:50:41]

https://codereview.chromium.org/1651163002/diff/80001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/1651163002/diff/80001/src/IceDefs.h#newcode313
src/IceDefs.h:313: using RelocOffsetArray = llvm::SmallVector<RelocOffset *, 4>;
On 2016/02/02 20:57:49, stichnot wrote:
> You could use "class RelocOffset" here and remove the explicit forward
> declaration above, if you prefer.

Done.

https://codereview.chromium.org/1651163002/diff/80001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1651163002/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:50: if (Key.OffsetExpr[0]->hasOffset()) {
On 2016/02/02 20:57:49, stichnot wrote:
> Please assert(!Key.OffsetExpr.empty())
> otherwise it looks like a bug.

Done.

https://codereview.chromium.org/1651163002/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:55: hash<std::size_t>()(Key.OffsetExpr.size());
On 2016/02/02 20:57:49, stichnot wrote:
> remove this

I meant to add it to the return value.

https://codereview.chromium.org/1651163002/diff/80001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/1651163002/diff/80001/src/IceOperand.h#newcode29
src/IceOperand.h:29: #include <limits>
On 2016/02/02 20:57:49, stichnot wrote:
> I don't think this is actually necessary, is it?

well, numeric_limits appear in this file. leaving it.

https://codereview.chromium.org/1651163002/diff/80001/src/IceOperand.h#newcod...
src/IceOperand.h:365: const IceString EmitString;        /// optional for
textual output
On 2016/02/02 20:57:49, stichnot wrote:
> I would change "output" to "asm" or "emission"

Done.