Insert denominator zero checks for NaCl

Insert denominator zero checks for NaCl

This IR pass for ARM inserts a comparison and a branch to trap if the
denominator of a DIV or REM instruction is zero.  This makes ARM fault
identically to x86 in this case.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=2833
R=eliben@chromium.org

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

[sehr, 2013-05-02 21:16:52]

PTAL.

[Jim Stichnoth, 2013-05-02 21:37:16]

https://codereview.chromium.org/14607004/diff/1/lib/Target/ARM/ARMNaClDivideC...
File lib/Target/ARM/ARMNaClDivideCheck.cpp (right):

https://codereview.chromium.org/14607004/diff/1/lib/Target/ARM/ARMNaClDivideC...
lib/Target/ARM/ARMNaClDivideCheck.cpp:61: // Create a single trap block.
You might want to check whether the denominator is a nonzero constant and avoid
the splitting.  This may be true reasonably often.

[eliben, 2013-05-02 21:47:31]

Add tests, and also Mark and/or Derek should take a look since they've written a
bunch of passes lately

https://codereview.chromium.org/14607004/diff/1/lib/Target/ARM/ARMNaClDivideC...
File lib/Target/ARM/ARMNaClDivideCheck.cpp (right):

https://codereview.chromium.org/14607004/diff/1/lib/Target/ARM/ARMNaClDivideC...
lib/Target/ARM/ARMNaClDivideCheck.cpp:10: // This pass adds a check for divide
by zero after every integer DIV or REM.
after?

https://codereview.chromium.org/14607004/diff/1/lib/Target/ARM/ARMNaClDivideC...
lib/Target/ARM/ARMNaClDivideCheck.cpp:49: // BasicBlock list is being appended
to during the optimization.
I'd prefer to see a beefier comment explaining that the BB can be split in which
case the next iteration will go to the new block

https://codereview.chromium.org/14607004/diff/1/lib/Target/ARM/ARMNaClDivideC...
lib/Target/ARM/ARMNaClDivideCheck.cpp:62: if (TrapBlock == NULL) {
Put the trap block creation into a helper method?

https://codereview.chromium.org/14607004/diff/1/lib/Target/ARM/ARMNaClDivideC...
lib/Target/ARM/ARMNaClDivideCheck.cpp:64: &F, NULL);
No need for explicit NULL here

https://codereview.chromium.org/14607004/diff/1/lib/Target/ARM/ARMNaClDivideC...
lib/Target/ARM/ARMNaClDivideCheck.cpp:70: BasicBlock *Successor =
BB->splitBasicBlock(BI, "");
suggested tests: the div/rem is first / last instruction (obviously last before
terminator) in a BB

[sehr, 2013-05-03 20:33:06]

Optimization, tests, and review comments added.  PTAL.

https://codereview.chromium.org/14607004/diff/1/lib/Target/ARM/ARMNaClDivideC...
File lib/Target/ARM/ARMNaClDivideCheck.cpp (right):

https://codereview.chromium.org/14607004/diff/1/lib/Target/ARM/ARMNaClDivideC...
lib/Target/ARM/ARMNaClDivideCheck.cpp:10: // This pass adds a check for divide
by zero after every integer DIV or REM.
On 2013/05/02 21:47:31, eliben wrote:
> after?

I changed the code to put the branch before.

https://codereview.chromium.org/14607004/diff/1/lib/Target/ARM/ARMNaClDivideC...
lib/Target/ARM/ARMNaClDivideCheck.cpp:49: // BasicBlock list is being appended
to during the optimization.
On 2013/05/02 21:47:31, eliben wrote:
> I'd prefer to see a beefier comment explaining that the BB can be split in
which
> case the next iteration will go to the new block

Done.

https://codereview.chromium.org/14607004/diff/1/lib/Target/ARM/ARMNaClDivideC...
lib/Target/ARM/ARMNaClDivideCheck.cpp:61: // Create a single trap block.
On 2013/05/02 21:37:16, stichnot wrote:
> You might want to check whether the denominator is a nonzero constant and
avoid
> the splitting.  This may be true reasonably often.

Done.  I insert a trap before any explicit divide by zero and elide the test
before any divide by non-zero constant.

https://codereview.chromium.org/14607004/diff/1/lib/Target/ARM/ARMNaClDivideC...
lib/Target/ARM/ARMNaClDivideCheck.cpp:62: if (TrapBlock == NULL) {
On 2013/05/02 21:47:31, eliben wrote:
> Put the trap block creation into a helper method?

Done.

https://codereview.chromium.org/14607004/diff/1/lib/Target/ARM/ARMNaClDivideC...
lib/Target/ARM/ARMNaClDivideCheck.cpp:64: &F, NULL);
On 2013/05/02 21:47:31, eliben wrote:
> No need for explicit NULL here

Done.

https://codereview.chromium.org/14607004/diff/1/lib/Target/ARM/ARMNaClDivideC...
lib/Target/ARM/ARMNaClDivideCheck.cpp:70: BasicBlock *Successor =
BB->splitBasicBlock(BI, "");
On 2013/05/02 21:47:31, eliben wrote:
> suggested tests: the div/rem is first / last instruction (obviously last
before
> terminator) in a BB

Done.

[sehr, 2013-05-03 20:33:44]

More reviewers.  PTAL.

[eliben, 2013-05-03 21:18:55]

https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
File lib/Target/ARM/ARMNaClDivideCheck.cpp (right):

https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
lib/Target/ARM/ARMNaClDivideCheck.cpp:10: // This pass adds a check for divide
by zero before every integer DIV or REM.
Once this CL is committed, we can check the performance implication when running
spec with fastisel. If it's not negligible, we should try placing the check
*after* the div, or fixing the register allocator, whichever is easier ;-)

https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
lib/Target/ARM/ARMNaClDivideCheck.cpp:60: // DIV/REM into two blocks.  The new
BasicBlocks are added to the end of
I'm not sure I understand the "at the end of the function" part. You create two
types of blocks: one is the trap block which is only created once and we don't
care where it's placed. The other is the continuation block per split, but
that's decided for you by splitBasicBlock. splitBasicBlock IIUC inserts like
this:

BBA->BBB->BBC
      ^ 
   split here

BBA->BBB1->BBB2->BBC

https://codereview.chromium.org/14607004/diff/6001/test/NaCl/ARM/divrem-guard...
File test/NaCl/ARM/divrem-guards.ll (right):

https://codereview.chromium.org/14607004/diff/6001/test/NaCl/ARM/divrem-guard...
test/NaCl/ARM/divrem-guards.ll:1: ; RUN: llc -mtriple=armv7-unknown-nacl
-sfi-branch -filetype=obj %s -o - \
I wonder if there's a way to hook this pass to opt somehow so it could be tested
through it?

Since it's a IR->IR transform, such testing can be way more effective

https://codereview.chromium.org/14607004/diff/6001/test/NaCl/ARM/divrem-guard...
test/NaCl/ARM/divrem-guards.ll:8: %x.addr = alloca i32, align 4
Could you minimize this repeating test case to something smaller? i.e. you don't
need the allocas at all.

https://codereview.chromium.org/14607004/diff/6001/test/NaCl/ARM/divrem-guard...
test/NaCl/ARM/divrem-guards.ll:16: ; CHECK-NEXT: beq
I'd check that a trap exists too, before the next return or something

[Mark Seaborn, 2013-05-03 21:25:36]

https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
File lib/Target/ARM/ARMNaClDivideCheck.cpp (right):

https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
lib/Target/ARM/ARMNaClDivideCheck.cpp:1: //===- ARMNaClDivideCheck.cpp - Add
divide by zero checks ----------------===//
Add 1 more '-' to line up right-hand edge

https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
lib/Target/ARM/ARMNaClDivideCheck.cpp:33: 
Remove this empty line

https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
lib/Target/ARM/ARMNaClDivideCheck.cpp:35: 
Remove this empty line

https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
lib/Target/ARM/ARMNaClDivideCheck.cpp:79: if (DenomConst) {
Write as:
  if (ConstantInt *DenomConst = dyn_cast<ConstantInt>(Denominator)) { ...

https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
lib/Target/ARM/ARMNaClDivideCheck.cpp:85: CallInst::Create(TrapFn, "", DivInst);
Preserve debug location:
CallInst::Create(TrapFn, "", DivInst)->setDebugLoc(DivInst->getDebugLoc());

https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
lib/Target/ARM/ARMNaClDivideCheck.cpp:89: // Create a single trap block.
This could hinder debugging.  It would be nice to preserve debugging info if it
is present.

Maybe if DivInst.getDebugLoc() returns a location, we could create unique trap
blocks?  Or you could reuse a trap block between instructions if their debug
locations match?

Or if you're worried about speed, comment that we're not preserving debugging. 
Is there a flag we can check to see whether debugging is enabled?

https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
lib/Target/ARM/ARMNaClDivideCheck.cpp:95:
BB->getTerminator()->eraseFromParent();
Comment: "Remove unconditional branch"?

https://codereview.chromium.org/14607004/diff/6001/test/NaCl/ARM/divrem-guard...
File test/NaCl/ARM/divrem-guards.ll (right):

https://codereview.chromium.org/14607004/diff/6001/test/NaCl/ARM/divrem-guard...
test/NaCl/ARM/divrem-guards.ll:12: %0 = load i32* %x.addr, align 4
Remove the loads+stores.  You're copying Clang's -O0 output, right?  This can
become:

define i32 @mysdiv(i32 %x, i32 %y) {
  %div = sdiv i32 %x, %y
  ret i32 %div
}

https://codereview.chromium.org/14607004/diff/6001/test/NaCl/ARM/divrem-guard...
test/NaCl/ARM/divrem-guards.ll:15: ; CHECK: cmp r1, #0
It would be better to test at the IR level too.  Then you can have a
non-ARM-specific test.

[sehr, 2013-05-03 23:29:05]

I have moved the transformation to Transforms/NaCl.  This allowed calling it
from opt also, and I have augmented the test to do so.  PTAL.

https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
File lib/Target/ARM/ARMNaClDivideCheck.cpp (right):

https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
lib/Target/ARM/ARMNaClDivideCheck.cpp:1: //===- ARMNaClDivideCheck.cpp - Add
divide by zero checks ----------------===//
On 2013/05/03 21:25:36, Mark Seaborn wrote:
> Add 1 more '-' to line up right-hand edge

Done.

https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
lib/Target/ARM/ARMNaClDivideCheck.cpp:10: // This pass adds a check for divide
by zero before every integer DIV or REM.
On 2013/05/03 21:18:55, eliben wrote:
> Once this CL is committed, we can check the performance implication when
running
> spec with fastisel. If it's not negligible, we should try placing the check
> *after* the div, or fixing the register allocator, whichever is easier ;-)

I know fixing it here is simpler a priori.  But on ARMs that have to use
software DIV/REM, it probably won't make any difference anyway, since we have to
call a function to do the DIV/REM, which will imply a spill-reload pair.  So I
relent.

https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
lib/Target/ARM/ARMNaClDivideCheck.cpp:33: 
On 2013/05/03 21:25:36, Mark Seaborn wrote:
> Remove this empty line

Done.

https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
lib/Target/ARM/ARMNaClDivideCheck.cpp:35: 
On 2013/05/03 21:25:36, Mark Seaborn wrote:
> Remove this empty line

Done.

https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
lib/Target/ARM/ARMNaClDivideCheck.cpp:60: // DIV/REM into two blocks.  The new
BasicBlocks are added to the end of
On 2013/05/03 21:18:55, eliben wrote:
> I'm not sure I understand the "at the end of the function" part. You create
two
> types of blocks: one is the trap block which is only created once and we don't
> care where it's placed. The other is the continuation block per split, but
> that's decided for you by splitBasicBlock. splitBasicBlock IIUC inserts like
> this:
> 
> BBA->BBB->BBC
>       ^ 
>    split here
> 
> BBA->BBB1->BBB2->BBC

I changed it to just emphasize the important point -- that the blocks are
created *after* the current block being processed.

https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
lib/Target/ARM/ARMNaClDivideCheck.cpp:79: if (DenomConst) {
On 2013/05/03 21:25:36, Mark Seaborn wrote:
> Write as:
>   if (ConstantInt *DenomConst = dyn_cast<ConstantInt>(Denominator)) { ...

Done.

https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
lib/Target/ARM/ARMNaClDivideCheck.cpp:85: CallInst::Create(TrapFn, "", DivInst);
On 2013/05/03 21:25:36, Mark Seaborn wrote:
> Preserve debug location:
> CallInst::Create(TrapFn, "", DivInst)->setDebugLoc(DivInst->getDebugLoc());

Done.

https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
lib/Target/ARM/ARMNaClDivideCheck.cpp:89: // Create a single trap block.
On 2013/05/03 21:25:36, Mark Seaborn wrote:
> This could hinder debugging.  It would be nice to preserve debugging info if
it
> is present.
> 
> Maybe if DivInst.getDebugLoc() returns a location, we could create unique trap
> blocks?  Or you could reuse a trap block between instructions if their debug
> locations match?
> 
> Or if you're worried about speed, comment that we're not preserving debugging.

> Is there a flag we can check to see whether debugging is enabled?

I was only really interested in code size, and I guess I can go ahead and just
generate N blocks.

https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
lib/Target/ARM/ARMNaClDivideCheck.cpp:95:
BB->getTerminator()->eraseFromParent();
On 2013/05/03 21:25:36, Mark Seaborn wrote:
> Comment: "Remove unconditional branch"?

Done.

https://codereview.chromium.org/14607004/diff/6001/test/NaCl/ARM/divrem-guard...
File test/NaCl/ARM/divrem-guards.ll (right):

https://codereview.chromium.org/14607004/diff/6001/test/NaCl/ARM/divrem-guard...
test/NaCl/ARM/divrem-guards.ll:1: ; RUN: llc -mtriple=armv7-unknown-nacl
-sfi-branch -filetype=obj %s -o - \
On 2013/05/03 21:18:55, eliben wrote:
> I wonder if there's a way to hook this pass to opt somehow so it could be
tested
> through it?
> 
> Since it's a IR->IR transform, such testing can be way more effective

I have added an invocation under opt.

https://codereview.chromium.org/14607004/diff/6001/test/NaCl/ARM/divrem-guard...
test/NaCl/ARM/divrem-guards.ll:8: %x.addr = alloca i32, align 4
On 2013/05/03 21:18:55, eliben wrote:
> Could you minimize this repeating test case to something smaller? i.e. you
don't
> need the allocas at all.

Done.

https://codereview.chromium.org/14607004/diff/6001/test/NaCl/ARM/divrem-guard...
test/NaCl/ARM/divrem-guards.ll:12: %0 = load i32* %x.addr, align 4
On 2013/05/03 21:25:36, Mark Seaborn wrote:
> Remove the loads+stores.  You're copying Clang's -O0 output, right?  This can
> become:
> 
> define i32 @mysdiv(i32 %x, i32 %y) {
>   %div = sdiv i32 %x, %y
>   ret i32 %div
> }

Yes, removed.

https://codereview.chromium.org/14607004/diff/6001/test/NaCl/ARM/divrem-guard...
test/NaCl/ARM/divrem-guards.ll:15: ; CHECK: cmp r1, #0
On 2013/05/03 21:25:36, Mark Seaborn wrote:
> It would be better to test at the IR level too.  Then you can have a
> non-ARM-specific test.

I am now testing both ways.

https://codereview.chromium.org/14607004/diff/6001/test/NaCl/ARM/divrem-guard...
test/NaCl/ARM/divrem-guards.ll:15: ; CHECK: cmp r1, #0
On 2013/05/03 21:25:36, Mark Seaborn wrote:
> It would be better to test at the IR level too.  Then you can have a
> non-ARM-specific test.

I am now checking both.

https://codereview.chromium.org/14607004/diff/6001/test/NaCl/ARM/divrem-guard...
test/NaCl/ARM/divrem-guards.ll:16: ; CHECK-NEXT: beq
On 2013/05/03 21:18:55, eliben wrote:
> I'd check that a trap exists too, before the next return or something

Done.

[eliben, 2013-05-06 16:09:38]

On 2013/05/03 23:29:05, sehr1 wrote:
> I have moved the transformation to Transforms/NaCl.  This allowed calling it
> from opt also, and I have augmented the test to do so.  PTAL.
> 
>
https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
> File lib/Target/ARM/ARMNaClDivideCheck.cpp (right):
> 
>
https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
> lib/Target/ARM/ARMNaClDivideCheck.cpp:1: //===- ARMNaClDivideCheck.cpp - Add
> divide by zero checks ----------------===//
> On 2013/05/03 21:25:36, Mark Seaborn wrote:
> > Add 1 more '-' to line up right-hand edge
> 
> Done.
> 
>
https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
> lib/Target/ARM/ARMNaClDivideCheck.cpp:10: // This pass adds a check for divide
> by zero before every integer DIV or REM.
> On 2013/05/03 21:18:55, eliben wrote:
> > Once this CL is committed, we can check the performance implication when
> running
> > spec with fastisel. If it's not negligible, we should try placing the check
> > *after* the div, or fixing the register allocator, whichever is easier ;-)
> 
> I know fixing it here is simpler a priori.  But on ARMs that have to use
> software DIV/REM, it probably won't make any difference anyway, since we have
to
> call a function to do the DIV/REM, which will imply a spill-reload pair.  So I
> relent.
> 
>
https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
> lib/Target/ARM/ARMNaClDivideCheck.cpp:33: 
> On 2013/05/03 21:25:36, Mark Seaborn wrote:
> > Remove this empty line
> 
> Done.
> 
>
https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
> lib/Target/ARM/ARMNaClDivideCheck.cpp:35: 
> On 2013/05/03 21:25:36, Mark Seaborn wrote:
> > Remove this empty line
> 
> Done.
> 
>
https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
> lib/Target/ARM/ARMNaClDivideCheck.cpp:60: // DIV/REM into two blocks.  The new
> BasicBlocks are added to the end of
> On 2013/05/03 21:18:55, eliben wrote:
> > I'm not sure I understand the "at the end of the function" part. You create
> two
> > types of blocks: one is the trap block which is only created once and we
don't
> > care where it's placed. The other is the continuation block per split, but
> > that's decided for you by splitBasicBlock. splitBasicBlock IIUC inserts like
> > this:
> > 
> > BBA->BBB->BBC
> >       ^ 
> >    split here
> > 
> > BBA->BBB1->BBB2->BBC
> 
> I changed it to just emphasize the important point -- that the blocks are
> created *after* the current block being processed.
> 
>
https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
> lib/Target/ARM/ARMNaClDivideCheck.cpp:79: if (DenomConst) {
> On 2013/05/03 21:25:36, Mark Seaborn wrote:
> > Write as:
> >   if (ConstantInt *DenomConst = dyn_cast<ConstantInt>(Denominator)) { ...
> 
> Done.
> 
>
https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
> lib/Target/ARM/ARMNaClDivideCheck.cpp:85: CallInst::Create(TrapFn, "",
DivInst);
> On 2013/05/03 21:25:36, Mark Seaborn wrote:
> > Preserve debug location:
> > CallInst::Create(TrapFn, "", DivInst)->setDebugLoc(DivInst->getDebugLoc());
> 
> Done.
> 
>
https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
> lib/Target/ARM/ARMNaClDivideCheck.cpp:89: // Create a single trap block.
> On 2013/05/03 21:25:36, Mark Seaborn wrote:
> > This could hinder debugging.  It would be nice to preserve debugging info if
> it
> > is present.
> > 
> > Maybe if DivInst.getDebugLoc() returns a location, we could create unique
trap
> > blocks?  Or you could reuse a trap block between instructions if their debug
> > locations match?
> > 
> > Or if you're worried about speed, comment that we're not preserving
debugging.
> 
> > Is there a flag we can check to see whether debugging is enabled?
> 
> I was only really interested in code size, and I guess I can go ahead and just
> generate N blocks.
> 
>
https://codereview.chromium.org/14607004/diff/6001/lib/Target/ARM/ARMNaClDivi...
> lib/Target/ARM/ARMNaClDivideCheck.cpp:95:
> BB->getTerminator()->eraseFromParent();
> On 2013/05/03 21:25:36, Mark Seaborn wrote:
> > Comment: "Remove unconditional branch"?
> 
> Done.
> 
>
https://codereview.chromium.org/14607004/diff/6001/test/NaCl/ARM/divrem-guard...
> File test/NaCl/ARM/divrem-guards.ll (right):
> 
>
https://codereview.chromium.org/14607004/diff/6001/test/NaCl/ARM/divrem-guard...
> test/NaCl/ARM/divrem-guards.ll:1: ; RUN: llc -mtriple=armv7-unknown-nacl
> -sfi-branch -filetype=obj %s -o - \
> On 2013/05/03 21:18:55, eliben wrote:
> > I wonder if there's a way to hook this pass to opt somehow so it could be
> tested
> > through it?
> > 
> > Since it's a IR->IR transform, such testing can be way more effective
> 
> I have added an invocation under opt.
> 
>
https://codereview.chromium.org/14607004/diff/6001/test/NaCl/ARM/divrem-guard...
> test/NaCl/ARM/divrem-guards.ll:8: %x.addr = alloca i32, align 4
> On 2013/05/03 21:18:55, eliben wrote:
> > Could you minimize this repeating test case to something smaller? i.e. you
> don't
> > need the allocas at all.
> 
> Done.
> 
>
https://codereview.chromium.org/14607004/diff/6001/test/NaCl/ARM/divrem-guard...
> test/NaCl/ARM/divrem-guards.ll:12: %0 = load i32* %x.addr, align 4
> On 2013/05/03 21:25:36, Mark Seaborn wrote:
> > Remove the loads+stores.  You're copying Clang's -O0 output, right?  This
can
> > become:
> > 
> > define i32 @mysdiv(i32 %x, i32 %y) {
> >   %div = sdiv i32 %x, %y
> >   ret i32 %div
> > }
> 
> Yes, removed.
> 
>
https://codereview.chromium.org/14607004/diff/6001/test/NaCl/ARM/divrem-guard...
> test/NaCl/ARM/divrem-guards.ll:15: ; CHECK: cmp r1, #0
> On 2013/05/03 21:25:36, Mark Seaborn wrote:
> > It would be better to test at the IR level too.  Then you can have a
> > non-ARM-specific test.
> 
> I am now testing both ways.
> 
>
https://codereview.chromium.org/14607004/diff/6001/test/NaCl/ARM/divrem-guard...
> test/NaCl/ARM/divrem-guards.ll:15: ; CHECK: cmp r1, #0
> On 2013/05/03 21:25:36, Mark Seaborn wrote:
> > It would be better to test at the IR level too.  Then you can have a
> > non-ARM-specific test.
> 
> I am now checking both.
> 
>
https://codereview.chromium.org/14607004/diff/6001/test/NaCl/ARM/divrem-guard...
> test/NaCl/ARM/divrem-guards.ll:16: ; CHECK-NEXT: beq
> On 2013/05/03 21:18:55, eliben wrote:
> > I'd check that a trap exists too, before the next return or something
> 
> Done.

Looking at the tests again, they all are very trivial with just single BB and a
single div. To be on the safe side, I'd add a test or two with a hairier CFG:

* A div in a BB which is neither first nor last in a function
* A div in a BB with more than one incoming edge
* Two divs in the same BB 
* Two divs in subsequent BBs

These can be combined or separate, your choice.

Otherwise LGTM

[Derek Schuff, 2013-05-06 16:18:49]

https://codereview.chromium.org/14607004/diff/14001/tools/opt/opt.cpp
File tools/opt/opt.cpp (right):

https://codereview.chromium.org/14607004/diff/14001/tools/opt/opt.cpp#newcode589
tools/opt/opt.cpp:589: initializeInsertDivideCheckPass(Registry);
we want to add an initialization for this pass in LLC too

[sehr, 2013-05-07 23:53:17]

More complex test added.  PTAL.

https://codereview.chromium.org/14607004/diff/14001/tools/opt/opt.cpp
File tools/opt/opt.cpp (right):

https://codereview.chromium.org/14607004/diff/14001/tools/opt/opt.cpp#newcode589
tools/opt/opt.cpp:589: initializeInsertDivideCheckPass(Registry);
On 2013/05/06 16:18:49, Derek Schuff wrote:
> we want to add an initialization for this pass in LLC too

As we chatted about -- I am inserting a call to create the optimization in the
ARMTargetMachine.

[eliben, 2013-05-08 15:26:53]

lgtm

[sehr, 2013-05-08 16:28:35]

Committed patchset #4 manually as r8c9803a (presubmit successful).

[jvoung (off chromium), 2013-05-08 18:36:43]

Does lib/Target/ARM need to depend on the NaClTransforms library now? 
(somewhere in the LLVMBuild.txt and/or CMakeLists.txt)

I'm seeing:

.../pnacl/build/translator-armv7/llvm-sb/Release/lib/libLLVMARMCodeGen.a: error:
undefined reference to 'llvm::createInsertDivideCheckPass()'