569753 - nanojit: clean up ExprFilter::ins2()

Status: RESOLVED FIXED

(Core Graveyard :: Nanojit, defect)

Description

[Nicholas Nethercote [inactive]]

ExprFilter:ins2() is 230 lines long, and really hard to read.  It should be split into multiple functions.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: patch

I tried splitting it into multiple functions but it ended up being no easier
to read.  So I've done some cleanups within the single big function:

- Tweaked spacing, esp. put numerous case statements on the same line as the
  case label, which makes them easier to read and lets you see more code at
  once.
  
- Fixed up some inconsistent and unnecessary uses of int32_t and uint32_t 
  casts.
  
- Added some comments.

One question:  we have this "fold" case for constant +/-/* expressions where
we carefully avoid overflow.  Is this worth bothering with?  Will the 
compile-time overflow not be equivalent to the run-time overflow?  Maybe 
it's worth keeping just to be prudent, but I want to understand if there are
cases where it's truly necessary to avoid changing the program's meaning.

Comment 2

[Edwin Smith]

Comment on attachment 450291 [details] [diff] [review]
patch

I think the overflow avoidance was a holdover from the LIR_ov days.  if the expression was constant then the add could be removed, leaving the LIR_ov hanging around with no input.  

Now, with our new LIR_*ov* variants, seems like we can let the plain operators overflow as long as the C compilers do what our jit would do.  safe assumption i think, especially since we're not cross compiling.  I dont think the C compilers will be able to exploit C's loopholes that overflows are undefined, because it can't see the values as constants.

R+ for style, I'm assuming you're testing LIR diffs so I havent reviewed carefully for typo-style injections.

Comment 3

[Steven Johnson]

style q: are we officially preferring 

   if (condition) {

over

   if (condition) 
   {
   
in nanojit? (IMHO the latter makes for easier to read code on modern giganto-screens, and is more prevalent in TR code, but I'm not religious about it...)

Comment 4

[Nicholas Nethercote [inactive]]

NJ: http://hg.mozilla.org/projects/nanojit-central/rev/27e933f40d36
TM: http://hg.mozilla.org/tracemonkey/rev/895516ddc0be

Comment 5

[Nicholas Nethercote [inactive]]

(In reply to comment #2)
> (From update of attachment 450291 [details] [diff] [review])
> I think the overflow avoidance was a holdover from the LIR_ov days.  if the
> expression was constant then the add could be removed, leaving the LIR_ov
> hanging around with no input.  
> 
> Now, with our new LIR_*ov* variants, seems like we can let the plain operators
> overflow as long as the C compilers do what our jit would do.  safe assumption
> i think, especially since we're not cross compiling.  I dont think the C
> compilers will be able to exploit C's loopholes that overflows are undefined,
> because it can't see the values as constants.

I left the overflow checking in (paranoia) and added a comment.


(In reply to comment #3)
> style q: are we officially preferring 
> 
>    if (condition) {
> 
> over
> 
>    if (condition) 
>    {
> 
> in nanojit?

Let me consult the NJ style guide... oh, there isn't one :)  I prefer the former and so tend to use it.  Unless we initiate a style guide I think there will always be inconsistencies.

Comment 6

[Edwin Smith]

TR: http://hg.mozilla.org/tamarin-redux/rev/3d4bf0e0a2cd

Comment 7

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/895516ddc0be