Iasm and obj lowering for advanced switch lowering.

Iasm and obj lowering for advanced switch lowering.

Jump table emission is delayed until offsets are known. X86 local jumps can be
near or far. Sanboxing is applied to indirect jumps from jump table.

BUG=
R=stichnot@chromium.org, jvoung

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

[ascull, 2015-07-28 22:30:46]

https://codereview.chromium.org/1257283004/diff/1/src/IceClFlags.cpp
File src/IceClFlags.cpp (right):

https://codereview.chromium.org/1257283004/diff/1/src/IceClFlags.cpp#newcode426
src/IceClFlags.cpp:426:
OutFlags.setUseAdvancedSwitchLowering(::UseAdvancedSwitchLowering);
No need for this. Done.

[ascull, 2015-07-29 19:46:58]

ascull@google.com changed reviewers:
+ stichnot@chromium.org

[ascull, 2015-07-29 19:46:59]

Advanced switch lowering works for asm, iasm and obj. It also works in the
sandbox. Some cases, such as 64-bit, are not fully implemented as they will be
better approached from the (future) high level lowering.

Performance measurements have been erratic so its hard to tell what actually
happens. It doesn't appear there are any regressions and a few benchmarks
benefit by a few of percent.

[jvoung (off chromium), 2015-07-29 21:31:17]

https://codereview.chromium.org/1257283004/diff/1/src/IceAssembler.h
File src/IceAssembler.h (right):

https://codereview.chromium.org/1257283004/diff/1/src/IceAssembler.h#newcode236
src/IceAssembler.h:236: virtual void alignCfgNode() = 0;
You might be able to make this non-virtual if it can be implemented by
padWithNop(X), where X is calculated from the Buffer.getPosition() vs
getBundleAlignLog2Bytes().

https://codereview.chromium.org/1257283004/diff/1/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/1257283004/diff/1/src/IceCfg.h#newcode242
src/IceCfg.h:242: /// called once all repointing has taken palce.
palce -> place

https://codereview.chromium.org/1257283004/diff/1/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/1257283004/diff/1/src/IceGlobalContext.h#newc...
src/IceGlobalContext.h:219: JumpTableData &addJumpTable(IceString FuncName,
SizeT Id, SizeT NumTargets) {
Maybe outline this instead of having it inline in the .h.

https://codereview.chromium.org/1257283004/diff/1/src/IceSwitchLowering.h
File src/IceSwitchLowering.h (right):

https://codereview.chromium.org/1257283004/diff/1/src/IceSwitchLowering.h#new...
src/IceSwitchLowering.h:83: /// section.
clarify ELF section ?

https://codereview.chromium.org/1257283004/diff/1/src/IceSwitchLowering.h#new...
src/IceSwitchLowering.h:100: std::vector<intptr_t> getTargetOffsets() const {
return TargetOffsets; }
can this return a vector ref instead?

https://codereview.chromium.org/1257283004/diff/20001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/1257283004/diff/20001/src/IceCfg.cpp#newcode617
src/IceCfg.cpp:617: Str << "\t" << Asm->getNonExecPadDirective() << " "
Maybe just rename this from getNonExecPadDirective() -> getAlignDirective()?

For functions we insert NonExec bytes, and need to specify (a) which bytes to
use, and (b) how big an instruction those bytes are (1 byte, 2 byte, 4 byte).
Here, between basic blocks we have executable padding, which is the default from
p2align, so "NonExec" is a bit misleading.

https://codereview.chromium.org/1257283004/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1257283004/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86Base.h:163: /// Lower and indirect jump adding
sandboxing when needed.
"Lower an indirect" ?

https://codereview.chromium.org/1257283004/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1257283004/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:458: }
// Mark nodes that require sandbox alignment
  if (Ctx->getFlags().getUseSandboxing())
    Func->markNodesForSandboxing();

For Om1 also? The jumptable lowering isn't restricted to O2 right now is it?

https://codereview.chromium.org/1257283004/diff/20001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/switch-opt.ll (left):

https://codereview.chromium.org/1257283004/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/switch-opt.ll:5: ; RUN: %p2i -i %s --filetype=obj
--disassemble --args -O2 | FileCheck %s
Why remove this?

[ascull, 2015-07-29 22:43:05]

https://codereview.chromium.org/1257283004/diff/1/src/IceAssembler.h
File src/IceAssembler.h (right):

https://codereview.chromium.org/1257283004/diff/1/src/IceAssembler.h#newcode236
src/IceAssembler.h:236: virtual void alignCfgNode() = 0;
On 2015/07/29 21:31:16, jvoung wrote:
> You might be able to make this non-virtual if it can be implemented by
> padWithNop(X), where X is calculated from the Buffer.getPosition() vs
> getBundleAlignLog2Bytes().

Done.

https://codereview.chromium.org/1257283004/diff/1/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/1257283004/diff/1/src/IceCfg.h#newcode242
src/IceCfg.h:242: /// called once all repointing has taken palce.
On 2015/07/29 21:31:16, jvoung wrote:
> palce -> place

Done.

https://codereview.chromium.org/1257283004/diff/1/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/1257283004/diff/1/src/IceGlobalContext.h#newc...
src/IceGlobalContext.h:219: JumpTableData &addJumpTable(IceString FuncName,
SizeT Id, SizeT NumTargets) {
On 2015/07/29 21:31:16, jvoung wrote:
> Maybe outline this instead of having it inline in the .h.

Done.

https://codereview.chromium.org/1257283004/diff/1/src/IceSwitchLowering.h
File src/IceSwitchLowering.h (right):

https://codereview.chromium.org/1257283004/diff/1/src/IceSwitchLowering.h#new...
src/IceSwitchLowering.h:83: /// section.
On 2015/07/29 21:31:16, jvoung wrote:
> clarify ELF section ?

Done.

https://codereview.chromium.org/1257283004/diff/1/src/IceSwitchLowering.h#new...
src/IceSwitchLowering.h:100: std::vector<intptr_t> getTargetOffsets() const {
return TargetOffsets; }
On 2015/07/29 21:31:16, jvoung wrote:
> can this return a vector ref instead?

Done.

https://codereview.chromium.org/1257283004/diff/20001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/1257283004/diff/20001/src/IceCfg.cpp#newcode617
src/IceCfg.cpp:617: Str << "\t" << Asm->getNonExecPadDirective() << " "
On 2015/07/29 21:31:17, jvoung wrote:
> Maybe just rename this from getNonExecPadDirective() -> getAlignDirective()?
> 
> For functions we insert NonExec bytes, and need to specify (a) which bytes to
> use, and (b) how big an instruction those bytes are (1 byte, 2 byte, 4 byte).
> Here, between basic blocks we have executable padding, which is the default
from
> p2align, so "NonExec" is a bit misleading.

Done.

https://codereview.chromium.org/1257283004/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1257283004/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86Base.h:163: /// Lower and indirect jump adding
sandboxing when needed.
On 2015/07/29 21:31:17, jvoung wrote:
> "Lower an indirect" ?

Done.

https://codereview.chromium.org/1257283004/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1257283004/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:458: }
On 2015/07/29 21:31:17, jvoung wrote:
> // Mark nodes that require sandbox alignment
>   if (Ctx->getFlags().getUseSandboxing())
>     Func->markNodesForSandboxing();
> 
> For Om1 also? The jumptable lowering isn't restricted to O2 right now is it?

Done.

https://codereview.chromium.org/1257283004/diff/20001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/switch-opt.ll (left):

https://codereview.chromium.org/1257283004/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/switch-opt.ll:5: ; RUN: %p2i -i %s --filetype=obj
--disassemble --args -O2 | FileCheck %s
On 2015/07/29 21:31:17, jvoung wrote:
> Why remove this?

Since there is no longer an --adv-switch flag, x86 does not lower in the same
way that this test expects. adv-switch-opt is the suitable test for x86 but ARM
still uses the old lowering so this test is needed for that.

The CHECK*s no longer work so I have removed those too.

[Jim Stichnoth, 2015-07-30 15:20:24]

https://codereview.chromium.org/1257283004/diff/40001/src/IceAssembler.h
File src/IceAssembler.h (right):

https://codereview.chromium.org/1257283004/diff/40001/src/IceAssembler.h#newc...
src/IceAssembler.h:51: static const uint32_t kWordSize = sizeof(uint32_t);
Make this private or protected?  constexpr?

https://codereview.chromium.org/1257283004/diff/40001/src/IceAssemblerX86Base.h
File src/IceAssemblerX86Base.h (right):

https://codereview.chromium.org/1257283004/diff/40001/src/IceAssemblerX86Base...
src/IceAssemblerX86Base.h:156: void BindLocalLabel(SizeT Number);
Change this to lowercase as well?

https://codereview.chromium.org/1257283004/diff/40001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/1257283004/diff/40001/src/IceCfg.cpp#newcode616
src/IceCfg.cpp:616: if (NeedSandboxing && Node->needsAlignment()) {
Guard the entire loop with a Ctx->getFlags().getUseSandboxing() test.
(also in emitIAS)

https://codereview.chromium.org/1257283004/diff/40001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/1257283004/diff/40001/src/IceCfg.h#newcode27
src/IceCfg.h:27: class InstJumpTable;
There are already a bunch of forward class declarations in IceDefs.h.  Does it
make sense to add this there instead?

https://codereview.chromium.org/1257283004/diff/40001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/1257283004/diff/40001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:472: std::vector<JumpTableData> JumpTables;
Make vector<JumpTableData> into a typedef, like JumpTableDataList.

https://codereview.chromium.org/1257283004/diff/40001/src/IceInstX86Base.h
File src/IceInstX86Base.h (right):

https://codereview.chromium.org/1257283004/diff/40001/src/IceInstX86Base.h#ne...
src/IceInstX86Base.h:376: const BrKind Kind;
Consider changing the field name to something more specific, like "Mode"?

Because my first assumption on what "kind" means with respect to a branch
instruction would probably be more like "conditional vs unconditional".

https://codereview.chromium.org/1257283004/diff/40001/src/IceSwitchLowering.h
File src/IceSwitchLowering.h (right):

https://codereview.chromium.org/1257283004/diff/40001/src/IceSwitchLowering.h...
src/IceSwitchLowering.h:22: class Cfg;
At least Cfg and CfgNode forward declarations should already be in IceDefs.h.

https://codereview.chromium.org/1257283004/diff/40001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1257283004/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4541: if (DefaultTarget == nullptr) {
How about something like
  if (DefaultTarget) {
    ...
  } else {
    ...
  }

I don't know if written style guides cover this, but I'm fine with "if (A)" as
shorthand for "if (A != nullptr)", though not so happy with "if (!A)" as
shorthand for "if (A == nullptr)".

https://codereview.chromium.org/1257283004/diff/40001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/switch-opt.ll (right):

https://codereview.chromium.org/1257283004/diff/40001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/switch-opt.ll:5: ; For x86 see adv-switch-lowering.ll
adv-switch-opt.ll

[ascull, 2015-07-30 17:30:00]

https://codereview.chromium.org/1257283004/diff/40001/src/IceAssembler.h
File src/IceAssembler.h (right):

https://codereview.chromium.org/1257283004/diff/40001/src/IceAssembler.h#newc...
src/IceAssembler.h:51: static const uint32_t kWordSize = sizeof(uint32_t);
On 2015/07/30 15:20:23, stichnot wrote:
> Make this private or protected?  constexpr?

Done.

https://codereview.chromium.org/1257283004/diff/40001/src/IceAssemblerX86Base.h
File src/IceAssemblerX86Base.h (right):

https://codereview.chromium.org/1257283004/diff/40001/src/IceAssemblerX86Base...
src/IceAssemblerX86Base.h:156: void BindLocalLabel(SizeT Number);
On 2015/07/30 15:20:23, stichnot wrote:
> Change this to lowercase as well?

Done.

https://codereview.chromium.org/1257283004/diff/40001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/1257283004/diff/40001/src/IceCfg.cpp#newcode616
src/IceCfg.cpp:616: if (NeedSandboxing && Node->needsAlignment()) {
On 2015/07/30 15:20:23, stichnot wrote:
> Guard the entire loop with a Ctx->getFlags().getUseSandboxing() test.
> (also in emitIAS)

This only wants to align if sandboxing is neede but it should alway emit. Do you
mean to have two versions on the loop, one with sandboxing and one without?

https://codereview.chromium.org/1257283004/diff/40001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/1257283004/diff/40001/src/IceCfg.h#newcode27
src/IceCfg.h:27: class InstJumpTable;
On 2015/07/30 15:20:23, stichnot wrote:
> There are already a bunch of forward class declarations in IceDefs.h.  Does it
> make sense to add this there instead?

Done.

https://codereview.chromium.org/1257283004/diff/40001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/1257283004/diff/40001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:472: std::vector<JumpTableData> JumpTables;
On 2015/07/30 15:20:23, stichnot wrote:
> Make vector<JumpTableData> into a typedef, like JumpTableDataList.

Despite my dislike of hiding the type, done.

https://codereview.chromium.org/1257283004/diff/40001/src/IceInstX86Base.h
File src/IceInstX86Base.h (right):

https://codereview.chromium.org/1257283004/diff/40001/src/IceInstX86Base.h#ne...
src/IceInstX86Base.h:376: const BrKind Kind;
On 2015/07/30 15:20:23, stichnot wrote:
> Consider changing the field name to something more specific, like "Mode"?
> 
> Because my first assumption on what "kind" means with respect to a branch
> instruction would probably be more like "conditional vs unconditional".

Done.

https://codereview.chromium.org/1257283004/diff/40001/src/IceSwitchLowering.h
File src/IceSwitchLowering.h (right):

https://codereview.chromium.org/1257283004/diff/40001/src/IceSwitchLowering.h...
src/IceSwitchLowering.h:22: class Cfg;
On 2015/07/30 15:20:24, stichnot wrote:
> At least Cfg and CfgNode forward declarations should already be in IceDefs.h.

Done.

https://codereview.chromium.org/1257283004/diff/40001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1257283004/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4541: if (DefaultTarget == nullptr) {
On 2015/07/30 15:20:24, stichnot wrote:
> How about something like
>   if (DefaultTarget) {
>     ...
>   } else {
>     ...
>   }
> 
> I don't know if written style guides cover this, but I'm fine with "if (A)" as
> shorthand for "if (A != nullptr)", though not so happy with "if (!A)" as
> shorthand for "if (A == nullptr)".

I have this way to match the `(DefaultTarget == nullptr)` below which places the
label.

https://codereview.chromium.org/1257283004/diff/40001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/switch-opt.ll (right):

https://codereview.chromium.org/1257283004/diff/40001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/switch-opt.ll:5: ; For x86 see adv-switch-lowering.ll
On 2015/07/30 15:20:24, stichnot wrote:
> adv-switch-opt.ll

Done.

[Jim Stichnoth, 2015-07-30 17:40:44]

Thanks -- can you upload the latest patchset?

https://codereview.chromium.org/1257283004/diff/40001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/1257283004/diff/40001/src/IceCfg.cpp#newcode616
src/IceCfg.cpp:616: if (NeedSandboxing && Node->needsAlignment()) {
On 2015/07/30 17:29:59, ascull wrote:
> On 2015/07/30 15:20:23, stichnot wrote:
> > Guard the entire loop with a Ctx->getFlags().getUseSandboxing() test.
> > (also in emitIAS)
> 
> This only wants to align if sandboxing is neede but it should alway emit. Do
you
> mean to have two versions on the loop, one with sandboxing and one without?

Whoops, I misread the code indentation, never mind...

https://codereview.chromium.org/1257283004/diff/40001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/1257283004/diff/40001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:472: std::vector<JumpTableData> JumpTables;
On 2015/07/30 17:29:59, ascull wrote:
> On 2015/07/30 15:20:23, stichnot wrote:
> > Make vector<JumpTableData> into a typedef, like JumpTableDataList.
> 
> Despite my dislike of hiding the type, done.

I share this dislike, but I find it less displeasing than when you decide to
change the container type, or use its ::iterator or ::value_type, or add an
allocator, etc.

https://codereview.chromium.org/1257283004/diff/40001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1257283004/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4541: if (DefaultTarget == nullptr) {
On 2015/07/30 17:30:00, ascull wrote:
> On 2015/07/30 15:20:24, stichnot wrote:
> > How about something like
> >   if (DefaultTarget) {
> >     ...
> >   } else {
> >     ...
> >   }
> > 
> > I don't know if written style guides cover this, but I'm fine with "if (A)"
as
> > shorthand for "if (A != nullptr)", though not so happy with "if (!A)" as
> > shorthand for "if (A == nullptr)".
> 
> I have this way to match the `(DefaultTarget == nullptr)` below which places
the
> label.

I see.

[ascull, 2015-07-30 17:42:16]

+patchset

[Jim Stichnoth, 2015-07-30 19:21:23]

https://codereview.chromium.org/1257283004/diff/40001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/1257283004/diff/40001/src/IceCfg.h#newcode27
src/IceCfg.h:27: class InstJumpTable;
On 2015/07/30 17:29:59, ascull wrote:
> On 2015/07/30 15:20:23, stichnot wrote:
> > There are already a bunch of forward class declarations in IceDefs.h.  Does
it
> > make sense to add this there instead?
> 
> Done.

But remove it here?

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

https://codereview.chromium.org/1257283004/diff/60001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:883: LockedPtr<std::vector<JumpTableData>> JumpTables =
getJumpTables();
JumpTableDataList

Or, use auto, which is already done in many/most uses of LockedPtr<> due to its
ugliness.

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

https://codereview.chromium.org/1257283004/diff/60001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:215: LockedPtr<std::vector<JumpTableData>>
getJumpTables() {
Change these to JumpTableDataList

[ascull, 2015-07-30 20:39:38]

https://codereview.chromium.org/1257283004/diff/40001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/1257283004/diff/40001/src/IceCfg.h#newcode27
src/IceCfg.h:27: class InstJumpTable;
On 2015/07/30 19:21:23, stichnot wrote:
> On 2015/07/30 17:29:59, ascull wrote:
> > On 2015/07/30 15:20:23, stichnot wrote:
> > > There are already a bunch of forward class declarations in IceDefs.h. 
Does
> it
> > > make sense to add this there instead?
> > 
> > Done.
> 
> But remove it here?

Done.

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

https://codereview.chromium.org/1257283004/diff/60001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:883: LockedPtr<std::vector<JumpTableData>> JumpTables =
getJumpTables();
On 2015/07/30 19:21:23, stichnot wrote:
> JumpTableDataList
> 
> Or, use auto, which is already done in many/most uses of LockedPtr<> due to
its
> ugliness.

Done.

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

https://codereview.chromium.org/1257283004/diff/60001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:215: LockedPtr<std::vector<JumpTableData>>
getJumpTables() {
On 2015/07/30 19:21:23, stichnot wrote:
> Change these to JumpTableDataList

Done.

[Jim Stichnoth, 2015-07-30 20:41:41]

lgtm

[ascull, 2015-07-30 20:54:49]

Committed patchset #6 (id:100001) manually as
86df4e9e6d183f07638440afd2c225b485c03917 (presubmit successful).