nanojit: isconstq() should only succeed for 64-bit integers

RESOLVED FIXED

Status

Core Graveyard
Nanojit
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin)

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

9 years ago
I am gradually distinguishing 64-bit ints and 64-bit floats more and more (see bug 534309).  isconstq() currently succeeds if the opcode is LIR_quad or LIR_float.  It should really just succeed for 64-bit ints;  we have isconstf() for 64-bit floats.

As part of this, it's probably worth renaming these functions, since I'm trying to get away from the "quad"/"q" terminology, tainted as it is by its confused history.  isImmI32()/isImmI64()/isImmF64()/isImmPtr() would match up with LTy, and the "Imm" is preferable to "const" IMHO (esp. since we already have lots of functions like imm32()).
(Assignee)

Comment 1

9 years ago
Bug 538514 should be fixed as a part of this bug.
(Assignee)

Updated

8 years ago
Duplicate of this bug: 538514
(Assignee)

Comment 3

8 years ago
I decided against doing the renaming in this bug, that can be part of bug 504506.
(Assignee)

Comment 4

8 years ago
Created attachment 434099 [details] [diff] [review]
NJ patch

This patch changes isconstq() to only succeed for 64-bit integers (previously it also succeeded for 64-bit floats), and does some other stuff to compensate.
Attachment #434099 - Flags: review?(rreitmai)
(Assignee)

Comment 5

8 years ago
Created attachment 434100 [details] [diff] [review]
NJ patch (right one this time)
Attachment #434099 - Attachment is obsolete: true
Attachment #434100 - Flags: review?(rreitmai)
Attachment #434099 - Flags: review?(rreitmai)
(Assignee)

Comment 6

8 years ago
Created attachment 434101 [details] [diff] [review]
TR patch

Only one change, but I'm not certain it's right.
Attachment #434101 - Flags: review?(rreitmai)
(Assignee)

Comment 7

8 years ago
Created attachment 434102 [details] [diff] [review]
TM patch

Nb: the first change is justified by the fact that f2i() should never return a 64-bit integer instruction.
Attachment #434102 - Flags: review?(dvander)

Comment 8

8 years ago
Comment on attachment 434101 [details] [diff] [review]
TR patch

looks right to me, the very next line calls imm64f().
Attachment #434101 - Flags: superreview+

Comment 9

8 years ago
Comment on attachment 434100 [details] [diff] [review]
NJ patch (right one this time)

(1) for reduced ifdef'ery,  isImmAny could be ' isconstqf() || isconst() '
(2) and in general can we get rid of the ifdefs; i.e. LIR_quad is 'on' always
(3) Lir.cpp conversions , are we able to be smarter for the conversions ?  e.g. 'LIR_i2f :  if ( oprnd->isconstf() ) return oprnd '
Attachment #434100 - Flags: review?(rreitmai) → review+

Updated

8 years ago
Attachment #434101 - Flags: review?(rreitmai) → review+
(Assignee)

Comment 10

8 years ago
(In reply to comment #9)
> (From update of attachment 434100 [details] [diff] [review])
> (1) for reduced ifdef'ery,  isImmAny could be ' isconstqf() || isconst() '

True, I'll change.

> (2) and in general can we get rid of the ifdefs; i.e. LIR_quad is 'on' always

I'm not keen on that.  There are a lot of 64-bit only opcodes, also some 32-bit only ones, some x86-only ones and some softfloat-only ones.  Until recently they weren't ifdef'd and the treatment of these not-always-used opcodes was inconsistent -- some places just used them (eg. in switch statements), some wrapped them in #ifdef, etc.

#ifdefs are ugly, but IMHO in this case the extra correctness assurance is worth it -- you can't try to handle an opcode that you shouldn't.  We have good testing coverage so all combinations get a workout (except softfloat, but that's a separate issue).  And it makes the final code size smaller.
(See bug 542326 comment 11 for more about this.)

If you disagree strongly can you open a new bug?  I don't think this bug is the place for this discussion.

> (3) Lir.cpp conversions , are we able to be smarter for the conversions ?  e.g.
> 'LIR_i2f :  if ( oprnd->isconstf() ) return oprnd '

I'm inclined to say no to that as well.  It'll happen in ExprFilter anyway, and it seems useful to be able to force particular LIR to be written without being optimised.  Eg. lirasm by default does not optimise code and so needs this facility.  Sound reasonable?
(Assignee)

Updated

8 years ago
Summary: nanojit: rename isconst(), isconstq(), isconstf(), isconstp() → nanojit: isconstq() should only succeed for 64-bit integers

Comment 11

8 years ago
(In reply to comment #10)
> (In reply to comment #9)
> If you disagree strongly can you open a new bug?  I don't think this bug is the
> place for this discussion.

I don't feel strongly on this point so no complaints which way we go.

> 
>   Sound reasonable?

Agreed, just raising it in case it didn't cross your mind.
Attachment #434102 - Flags: review?(dvander) → review+
(Assignee)

Comment 13

8 years ago
http://hg.mozilla.org/tracemonkey/rev/cb5914d2a5db
http://hg.mozilla.org/tracemonkey/rev/9d62aa689bd0
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey

Comment 14

8 years ago
http://hg.mozilla.org/tamarin-redux/rev/bd8233dc8d77
http://hg.mozilla.org/tamarin-redux/rev/0365d19900ff
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey → fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin

Comment 15

8 years ago
http://hg.mozilla.org/mozilla-central/rev/cb5914d2a5db
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Component: Nanojit → Nanojit
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.