Subzero: Improve/refactor folding loads into the next instruction.

Subzero: Improve/refactor folding loads into the next instruction.

This is turned into a separate (O2-only) pass that looks for opportunities:
1. A Load instruction, or an AtomicLoad intrinsic that would be lowered just like a Load instruction
2. Followed immediately by an instruction with a whitelisted kind that uses the Load dest variable as one of its operands
3. Where the whitelisted instruction ends the live range of the Load dest variable.

In such cases, the original two instructions are deleted and a new instruction is added that folds the load into the whitelisted instruction.

We also do some work to splice the liveness information (Inst::LiveRangesEnded and Inst::isLastUse()) into the new instruction, so that the target lowering pass might still take advantage.  Currently this is used quite sparingly, but in the future we could use that along with operator commutativity to choose among different lowering sequences to reduce register pressure.

The whitelisted instruction kinds are chosen based primarily on whether the main operation's native instruction can use a memory operand - e.g., arithmetic (add/sub/imul/etc), compare (cmp/ucomiss), cast (movsx/movzx/etc).  Notably, call and ret are not included because arg passing is done through simple assignments which normal lowering is sufficient for.

BUG= none
R=jvoung@chromium.org, mtrofin@chromium.org

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

[Jim Stichnoth, 2015-06-03 14:03:51]

stichnot@chromium.org changed reviewers:
+ jvoung@chromium.org, kschimpf@google.com, mtrofin@chromium.org

[Jim Stichnoth, 2015-06-03 14:14:50]

Verified that the scons tests pass with this patch, whereas they hit a Subzero
assertion failure with current master.
  ./scons -j32 platform=x86-32 bitcode=1 use_sz=1

[Mircea Trofin, 2015-06-03 14:47:17]

lgtm

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

https://codereview.chromium.org/1169493002/diff/1/src/IceInst.cpp
File src/IceInst.cpp (right):

https://codereview.chromium.org/1169493002/diff/1/src/IceInst.cpp#newcode140
src/IceInst.cpp:140: (RightMask >> (Index + getSrc(I)->getNumVars()));
Should this be RightMask << (Index + ...) ?

I notice RightMask is constructed from LRE >> (Index + 1), so I didn't think it
would shift right even more.

https://codereview.chromium.org/1169493002/diff/1/src/IceTargetLoweringX8632.cpp
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/1169493002/diff/1/src/IceTargetLoweringX8632....
src/IceTargetLoweringX8632.cpp:634: DoLegalize);
Was there anything that prevents an atomic load that's folded into Next w/ dead
dest from getting eliminated?

[Jim Stichnoth, 2015-06-03 21:06:51]

https://codereview.chromium.org/1169493002/diff/1/src/IceInst.cpp
File src/IceInst.cpp (right):

https://codereview.chromium.org/1169493002/diff/1/src/IceInst.cpp#newcode140
src/IceInst.cpp:140: (RightMask >> (Index + getSrc(I)->getNumVars()));
On 2015/06/03 18:18:15, jvoung wrote:
> Should this be RightMask << (Index + ...) ?
> 
> I notice RightMask is constructed from LRE >> (Index + 1), so I didn't think
it
> would shift right even more.

Oops, you're right!  Done.

https://codereview.chromium.org/1169493002/diff/1/src/IceTargetLoweringX8632.cpp
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/1169493002/diff/1/src/IceTargetLoweringX8632....
src/IceTargetLoweringX8632.cpp:634: DoLegalize);
On 2015/06/03 18:18:16, jvoung wrote:
> Was there anything that prevents an atomic load that's folded into Next w/
dead
> dest from getting eliminated?

Turns out nothing was being done, but it also turns out that prior liveness
analysis was already deleting instructions with a dead dest, so load folding
wouldn't be triggered.  For example:

define i32 @test_atomic_load_32_ignored(i32 %iptr) {
entry:
  %ptr = inttoptr i32 %iptr to i32*
  %ld = call i32 @llvm.nacl.atomic.load.i32(i32* %ptr, i32 6)
  %ignored = add i32 %iptr, %ld
  ret i32 0
}

Liveness analysis deletes the %ignored assignment, leaving the atomic load
instruction with nothing to be folded into.

In any case, I added the HasSideEffects management to Inst::spliceLastUseInfo(),
and renamed it to spliceLivenessInfo().

[jvoung (off chromium), 2015-06-03 21:14:05]

LGTM

https://codereview.chromium.org/1169493002/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/1169493002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:3107: InstLoad *Load = InstLoad::create(Func,
Dest, Instr->getArg(0));
Maybe at some point the load can just be tagged with side-effects, and the
FakeUse can be removed then?

[Jim Stichnoth, 2015-06-03 22:51:36]

https://codereview.chromium.org/1169493002/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/1169493002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:3107: InstLoad *Load = InstLoad::create(Func,
Dest, Instr->getArg(0));
On 2015/06/03 21:14:05, jvoung wrote:
> Maybe at some point the load can just be tagged with side-effects, and the
> FakeUse can be removed then?

This brings up a really good point.  The HasSideEffects flag is great for
preventing DCE in the high level ICE and in the low level instructions, but
there's a disconnect in the lowering phase where the high level side effects
info gets lost/ignored.

I added an issue to track this.
https://code.google.com/p/nativeclient/issues/detail?id=4193

[Jim Stichnoth, 2015-06-03 22:58:19]

Committed patchset #2 (id:20001) manually as
8e6bf6e113f496a80b900e61aec802196d83fb6d (tree was closed).