441872 - Introduce support for signed and unsigned overflow detection (LIR_ov and LIR_cs)

Status: RESOLVED FIXED

(Tamarin Graveyard :: Tracing Virtual Machine, defect)

Description

[Andreas Gal :gal]

This patch adds support for guarding and obtaining the boolean value of signed and unsigned overflow conditions. In contrast to LIR_eq and friends, LIR_ov and LIR_cs do not imply a CMP/TEST. Instead, LIR_ov and LIR_cs are placed immediately after the arithmetic operation we are interested in. The operand or LIR_ov and LIR_cs should point to this instruction for readability, but is otherwise unused (except during verbose mode).

Comment 1

[Andreas Gal :gal]

Attachment: Patch against TT tip.

It would be nice to ensure that LIR_ov and LIR_cs immediately precede the guard. I don't know how to assert that.

Comment 2

[Edwin Smith]

also need to update lirNames[] and operandCount[] tables.  

will it be a problem that isCmp() will be true for ov and cs?

to check that the next (backwards) instruction is what you want would require reading a head in the pipline, thats probably tricky in general.  

this would work if there arent any dead instructions inbetween that the pipeline has removed:

case LIR_xt:
  LirReader r(ins);
  LInsp next = r.previous();
  NanoAssert(next->isop(LIR_ov) || ... );

another idea is to put the assert in LIR_ov and LIR_cs, and require that the last assembled instruction was a guard.  that would be easy with a local variable in gen().

Comment 3

[Andreas Gal :gal]

We could have isCond() and isCmp(). isCmp() is the other once, isCond() is ov or cs or isCmp(). What do you think?

I don't think there is anything in there for xt/xt->lt either, so I will not worry about this assert for now but we should leave this bug open until we have one. This is going to be nasty to debug if someone gets it wrong higher up in the pipeline.

I wil fix up the other stuff and re-post.

Comment 4

[Andreas Gal :gal]

Attachment: Added Ed's fixes.

isCmp() is no longer true for ov and cs. isCond() includes isCmp() and ov and cs.

Comment 5

[Edwin Smith]

Comment on attachment 326807 [details] [diff] [review]
Added Ed's fixes.

testing now, will push soon assuming favorable outcome

Comment 6

[Edwin Smith]

Comment on attachment 326807 [details] [diff] [review]
Added Ed's fixes.

pushed as http://hg.mozilla.org/tamarin-tracing/index.cgi/rev/f315f22d8ed2

Comment 7

[Steven Johnson]

Patch is failing to compile on Debug ARM PocketPC builds for me

Comment 8

[Andreas Gal :gal]

Could you post the error message here? I don't have an arm platform to test with.

Comment 9

[Steven Johnson]

From the buildbot:

http://tamarin-builds.mozilla.org/tamarin-tracing/builders/windows/builds/49

1>..\..\nanojit\Assembler.cpp(888) : error C3861: 'MRNO': identifier not found

1>..\..\nanojit\Assembler.cpp(889) : error C3861: 'MRNC': identifier not found

1>..\..\nanojit\Assembler.cpp(1134) : error C3861: 'JNO': identifier not found

1>..\..\nanojit\Assembler.cpp(1136) : error C3861: 'JNC': identifier not found

1>..\..\nanojit\Assembler.cpp(1159) : error C3861: 'JO': identifier not found

1>..\..\nanojit\Assembler.cpp(1161) : error C3861: 'JC': identifier not found

1>..\..\nanojit\Assembler.cpp(1244) : error C3861: 'SETO': identifier not found

1>..\..\nanojit\Assembler.cpp(1246) : error C3861: 'SETC': identifier not found

Comment 10

[Edwin Smith]

fix pushed http://hg.mozilla.org/tamarin-tracing/index.cgi/rev/3b6524cc5ba4

Comment 11

[Andreas Gal :gal]

Attachment: Fix assert (should check for isCond not isCmp).

isCond is true for LIR instruction that modify the flags. isCmp only for the comparison op. This makes sure LIR_xf/xt check for isCond, not narrowly for isCmp.

Comment 12

[Andreas Gal :gal]

fix pushed http://hg.mozilla.org/index.cgi/tamarin-tracing/rev/f451ce11bcd4