ARM lowering integer divide and remainder, with div by 0 checks.

ARM lowering integer divide and remainder, with div by 0 checks.

ARM normally just returns 0 when dividing by 0 with the
software and hw implementations, which is different from
what X86 does. So, for NaCl, we've modified LLVM to trap
by inserting explicit 0 checks.

Uses -mattr=hwdiv-arm attribute to decide if 32-bit
sdiv/udiv are supported.

Also lower the unreachable-inst to a trap-inst, since we
need a trap instruction for divide by 0 anyway.

Misc: fix switch test under MINIMAL=1, since ARM requires
allow_dump for filetype=asm.
Random clang-format changes...

TODO: check via cross tests

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

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

[jvoung (off chromium), 2015-06-29 21:41:39]

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

[jvoung (off chromium), 2015-06-29 21:44:29]

https://codereview.chromium.org/1214693004/diff/40001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1214693004/diff/40001/src/IceInstARM32.h#newc...
src/IceInstARM32.h:265: Label,
Would it make sense to just make Label part of the high-level Ice?

[Jim Stichnoth, 2015-06-30 14:14:09]

otherwise lgtm

https://codereview.chromium.org/1214693004/diff/40001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1214693004/diff/40001/src/IceInstARM32.h#newc...
src/IceInstARM32.h:265: Label,
On 2015/06/29 21:44:28, jvoung wrote:
> Would it make sense to just make Label part of the high-level Ice?
> 

That seems reasonable.

https://codereview.chromium.org/1214693004/diff/100001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1214693004/diff/100001/src/IceInstARM32.h#new...
src/IceInstARM32.h:538: static InstARM32FourAddrGPR *create(Cfg *Func, Variable
*Dest, Variable *Src1,
Minor naming nit.  I think that we've been naming the src operands starting at
zero - unless Dest is implicitly Src0.  I.e., after the instruction is created,
getSrc(N) == Src<N> .

https://codereview.chromium.org/1214693004/diff/100001/src/IceInstARM32.h#new...
src/IceInstARM32.h:691: const InstARM32Label *NoLabel = nullptr;
constexpr?

https://codereview.chromium.org/1214693004/diff/100001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/1214693004/diff/100001/src/IceTargetLowering....
src/IceTargetLowering.h:356: const static constexpr char *H_sdiv_i32 =
"__divsi3";
Please update the "Other helper calls..." comment in runtime/szrt.c .

https://codereview.chromium.org/1214693004/diff/100001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1214693004/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:146: bool operandMustNotBeZero(const Operand *Op)
{
May want to tweak the function name to express the test against *integer* zero
as opposed to 0.0 .

Maybe something like isGuaranteedNonzeroInt() ?

https://codereview.chromium.org/1214693004/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1027: bool ElideCheck =
operandMustNotBeZero(SrcLo);
What do you think about starting the function like this?

  if (operandMustNotBeZero(SrcLo) || operandMustNotBeZero(SrcHi))
    return nullptr;

and then operandMustNotBeZero() returns false on a nullptr arg?  (perhaps by
changing dyn_cast<> to dyn_cast_or_null<>)

https://codereview.chromium.org/1214693004/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1314: const SizeT MaxSrcs = 2;
constexpr?

[jvoung (off chromium), 2015-06-30 16:58:08]

Thanks

https://codereview.chromium.org/1214693004/diff/40001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1214693004/diff/40001/src/IceInstARM32.h#newc...
src/IceInstARM32.h:265: Label,
On 2015/06/30 14:14:08, stichnot wrote:
> On 2015/06/29 21:44:28, jvoung wrote:
> > Would it make sense to just make Label part of the high-level Ice?
> > 
> 
> That seems reasonable.

Okay will try that in a separate CL.

https://codereview.chromium.org/1214693004/diff/100001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1214693004/diff/100001/src/IceInstARM32.h#new...
src/IceInstARM32.h:538: static InstARM32FourAddrGPR *create(Cfg *Func, Variable
*Dest, Variable *Src1,
On 2015/06/30 14:14:08, stichnot wrote:
> Minor naming nit.  I think that we've been naming the src operands starting at
> zero - unless Dest is implicitly Src0.  I.e., after the instruction is
created,
> getSrc(N) == Src<N> .

Ah -- Done here and for ThreeAddr also.

https://codereview.chromium.org/1214693004/diff/100001/src/IceInstARM32.h#new...
src/IceInstARM32.h:691: const InstARM32Label *NoLabel = nullptr;
On 2015/06/30 14:14:08, stichnot wrote:
> constexpr?

Done.

https://codereview.chromium.org/1214693004/diff/100001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/1214693004/diff/100001/src/IceTargetLowering....
src/IceTargetLowering.h:356: const static constexpr char *H_sdiv_i32 =
"__divsi3";
On 2015/06/30 14:14:09, stichnot wrote:
> Please update the "Other helper calls..." comment in runtime/szrt.c .

Done.

https://codereview.chromium.org/1214693004/diff/100001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1214693004/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:146: bool operandMustNotBeZero(const Operand *Op)
{
On 2015/06/30 14:14:09, stichnot wrote:
> May want to tweak the function name to express the test against *integer* zero
> as opposed to 0.0 .
> 
> Maybe something like isGuaranteedNonzeroInt() ?

Done.

https://codereview.chromium.org/1214693004/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1027: bool ElideCheck =
operandMustNotBeZero(SrcLo);
On 2015/06/30 14:14:09, stichnot wrote:
> What do you think about starting the function like this?
> 
>   if (operandMustNotBeZero(SrcLo) || operandMustNotBeZero(SrcHi))
>     return nullptr;
> 
> and then operandMustNotBeZero() returns false on a nullptr arg?  (perhaps by
> changing dyn_cast<> to dyn_cast_or_null<>)

Done.

https://codereview.chromium.org/1214693004/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1068: Context.insert(CheckLabel);
Oops, also realized that I need to do the trap, and jump over that instead.
Otherwise, the div code will fall through and also hit the trap (or would need
another jump to jump over it).

Changed this around to just have "div0Check" instead of "begin/end...".

Really need those execution crosstests...

https://codereview.chromium.org/1214693004/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1314: const SizeT MaxSrcs = 2;
On 2015/06/30 14:14:09, stichnot wrote:
> constexpr?

Done.

[Jim Stichnoth, 2015-06-30 17:06:51]

https://codereview.chromium.org/1214693004/diff/100001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1214693004/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1068: Context.insert(CheckLabel);
On 2015/06/30 16:58:08, jvoung wrote:
> Really need those execution crosstests...

Heh... I think those cross tests explicitly avoid dividing by zero.

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

Committed patchset #7 (id:120001) manually as
6ec369ebee62c3aab7fb7334d0bf3fac3a7133e5 (presubmit successful).