Add support for using MI bundles as bundle-locked groups on x86

Add support for using MI bundles as bundle-locked groups on x86

Use it to sandbox lea instructions with esp as the destination

[Derek Schuff, 2015-05-12 22:25:58]

dschuff@chromium.org changed reviewers:
+ jvoung@chromium.org

[Derek Schuff, 2015-05-12 22:25:59]

This on on the merge branch, where lea instructions changing %esp are being
generated.

Another option would be just to create MI pseudos for bundle_lock and unlock,
and insert them directly without bundling. Since X86NaClRewritePass is a
pre-emit pass, it should be safe from other passes moving the instructions
around (and in any case it is the last pass run).

[jvoung (off chromium), 2015-05-12 23:53:05]

https://codereview.chromium.org/1137803004/diff/20001/lib/Target/X86/X86MCIns...
File lib/Target/X86/X86MCInstLower.cpp (right):

https://codereview.chromium.org/1137803004/diff/20001/lib/Target/X86/X86MCIns...
lib/Target/X86/X86MCInstLower.cpp:1016: MBBI++;
++MBBI instead?

https://codereview.chromium.org/1137803004/diff/20001/lib/Target/X86/X86NaClR...
File lib/Target/X86/X86NaClRewritePass.cpp (right):

https://codereview.chromium.org/1137803004/diff/20001/lib/Target/X86/X86NaClR...
lib/Target/X86/X86NaClRewritePass.cpp:266: if (BaseReg != X86::EBP) {
Add a comment about why EBP is a special case?

Handled by NACL_SPADJ instead?

https://codereview.chromium.org/1137803004/diff/20001/lib/Target/X86/X86NaClR...
lib/Target/X86/X86NaClRewritePass.cpp:274: TII->get(X86::ADD64rr),
X86::RSP).addReg(X86::RSP).addReg(X86::R15);
80 col

https://codereview.chromium.org/1137803004/diff/20001/test/NaCl/X86/dynamic-s...
File test/NaCl/X86/dynamic-stack-alloc2.ll (right):

https://codereview.chromium.org/1137803004/diff/20001/test/NaCl/X86/dynamic-s...
test/NaCl/X86/dynamic-stack-alloc2.ll:3: ; CHECK-LABEL: @foo
nit: seems better to put this by the function (at least after the "target
datalayout ...; target triple ..." stuff in case more functions are added).

https://codereview.chromium.org/1137803004/diff/20001/test/NaCl/X86/dynamic-s...
test/NaCl/X86/dynamic-stack-alloc2.ll:12: define hidden void @foo(i32
%agg.result, <16 x i8> %v) {
The parameters are unused -- is it important for them to be around?

https://codereview.chromium.org/1137803004/diff/20001/test/NaCl/X86/dynamic-s...
test/NaCl/X86/dynamic-stack-alloc2.ll:14: br label %bb1
Is it required that the alloca not be in the entry block, or can this be
simplified more? Otherwise, document why?

https://codereview.chromium.org/1137803004/diff/20001/test/NaCl/X86/dynamic-s...
test/NaCl/X86/dynamic-stack-alloc2.ll:19: %gep109 = add i32 %.asint, 0
Would it be more resilient to optimization have add non-zero?

https://codereview.chromium.org/1137803004/diff/20001/test/NaCl/X86/dynamic-s...
test/NaCl/X86/dynamic-stack-alloc2.ll:30: 
extra space

[Derek Schuff, 2015-05-13 00:44:50]

https://codereview.chromium.org/1137803004/diff/20001/lib/Target/X86/X86MCIns...
File lib/Target/X86/X86MCInstLower.cpp (right):

https://codereview.chromium.org/1137803004/diff/20001/lib/Target/X86/X86MCIns...
lib/Target/X86/X86MCInstLower.cpp:1016: MBBI++;
On 2015/05/12 23:53:04, jvoung wrote:
> ++MBBI instead?

Done.

https://codereview.chromium.org/1137803004/diff/20001/lib/Target/X86/X86NaClR...
File lib/Target/X86/X86NaClRewritePass.cpp (right):

https://codereview.chromium.org/1137803004/diff/20001/lib/Target/X86/X86NaClR...
lib/Target/X86/X86NaClRewritePass.cpp:266: if (BaseReg != X86::EBP) {
On 2015/05/12 23:53:05, jvoung wrote:
> Add a comment about why EBP is a special case?
> 
> Handled by NACL_SPADJ instead?

Done.

https://codereview.chromium.org/1137803004/diff/20001/lib/Target/X86/X86NaClR...
lib/Target/X86/X86NaClRewritePass.cpp:274: TII->get(X86::ADD64rr),
X86::RSP).addReg(X86::RSP).addReg(X86::R15);
On 2015/05/12 23:53:04, jvoung wrote:
> 80 col

Done.

https://codereview.chromium.org/1137803004/diff/20001/test/NaCl/X86/dynamic-s...
File test/NaCl/X86/dynamic-stack-alloc2.ll (right):

https://codereview.chromium.org/1137803004/diff/20001/test/NaCl/X86/dynamic-s...
test/NaCl/X86/dynamic-stack-alloc2.ll:3: ; CHECK-LABEL: @foo
On 2015/05/12 23:53:05, jvoung wrote:
> nit: seems better to put this by the function (at least after the "target
> datalayout ...; target triple ..." stuff in case more functions are added).

Done.

https://codereview.chromium.org/1137803004/diff/20001/test/NaCl/X86/dynamic-s...
test/NaCl/X86/dynamic-stack-alloc2.ll:12: define hidden void @foo(i32
%agg.result, <16 x i8> %v) {
On 2015/05/12 23:53:05, jvoung wrote:
> The parameters are unused -- is it important for them to be around?

no, removed

https://codereview.chromium.org/1137803004/diff/20001/test/NaCl/X86/dynamic-s...
test/NaCl/X86/dynamic-stack-alloc2.ll:14: br label %bb1
On 2015/05/12 23:53:05, jvoung wrote:
> Is it required that the alloca not be in the entry block, or can this be
> simplified more? Otherwise, document why?

yes it is required, comment added.

https://codereview.chromium.org/1137803004/diff/20001/test/NaCl/X86/dynamic-s...
test/NaCl/X86/dynamic-stack-alloc2.ll:19: %gep109 = add i32 %.asint, 0
On 2015/05/12 23:53:05, jvoung wrote:
> Would it be more resilient to optimization have add non-zero?

actually the add isn't necessary at all.

https://codereview.chromium.org/1137803004/diff/20001/test/NaCl/X86/dynamic-s...
test/NaCl/X86/dynamic-stack-alloc2.ll:30: 
On 2015/05/12 23:53:05, jvoung wrote:
> extra space

Done.

[jvoung (off chromium), 2015-05-13 01:29:48]

lgtm

[Derek Schuff, 2015-05-13 15:59:39]

On 2015/05/13 01:29:48, jvoung wrote:
> lgtm

thanks, this is landed on the branch as
1ce51a013dd3a4ce41d2173cd19be93f7eb32731.
Will close the review manually.