Closed Bug 527512 Opened 10 years ago Closed 10 years ago

ExprFilter missing trivial optimizations for ugt, ult

Categories

(Core Graveyard :: Nanojit, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Future

People

(Reporter: stejohns, Assigned: stejohns)

References

Details

(Whiteboard: PACMAN, has-patch, fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin)

Attachments

(1 file, 3 obsolete files)

u32 < 0 -> always false
u32 > 0xffffffff -> always false
Attached patch Patch (obsolete) — Splinter Review
Attachment #411239 - Flags: review?(edwsmith)
Attachment #411239 - Flags: review?(edwsmith) → review+
Comment on attachment 411239 [details] [diff] [review]
Patch

only handles 32 bit, not 64bit, okay?

for u32 < 0, better to retrun oprnd2 since you know it's 0

this patch also misses cases for uge, presumably they're not important now.
(In reply to comment #2)
> (From update of attachment 411239 [details] [diff] [review])
> only handles 32 bit, not 64bit, okay?

heh, didn't realize there were 64-bit cases, I think those ops were added since I last hand my fingers in nanojit, will add and resubmit
 
> for u32 < 0, better to retrun oprnd2 since you know it's 0

gotcha

> this patch also misses cases for uge, presumably they're not important now.

ah, right, (unsigned >= 0) -> always true, will add
Attached patch Patch #2 (obsolete) — Splinter Review
Revisions per Edwin's comments.
Attachment #411239 - Attachment is obsolete: true
Attachment #411260 - Flags: review?(edwsmith)
Attachment #411260 - Flags: review?(nnethercote)
Attachment #411260 - Flags: review?(nnethercote) → review+
Comment on attachment 411260 [details] [diff] [review]
Patch #2

actually the 64bit cases won't run because the whole section of code is only running for 32bit constants.  but i'll tweak and push something adjusted.
Attachment #411260 - Flags: review?(edwsmith) → review+
Pretty sure that this has been pushed to all relevant repos, shall we close it?
Component: JIT Compiler (NanoJIT) → Nanojit
Product: Tamarin → Core
QA Contact: nanojit → nanojit
Target Milestone: --- → Future
AFAICT it has not landed.  assigning back to stejohns.
Assignee: nobody → stejohns
Severity: normal → minor
Whiteboard: PACMAN, has-patch
Attached patch Patch #3 (obsolete) — Splinter Review
Existing patch was hopelessly out of date (and pre-opcode-renaming), so offering updated and rebased patch for re-review.
Attachment #411260 - Attachment is obsolete: true
Attachment #447108 - Flags: review?(edwsmith)
Attachment #447108 - Flags: review?(nnethercote)
Attachment #447108 - Flags: review?(nnethercote) → review?(gal)
Comment on attachment 447108 [details] [diff] [review]
Patch #3

content wise patch looks fine.  style wise, use CASE64() instead of the ifdefs (seen elsewhere in LIR.cpp).  I'm switching the review to gal since since njn said he'd be out for a while.
Attachment #447108 - Flags: review?(edwsmith) → review+
Comment on attachment 447108 [details] [diff] [review]
Patch #3

>diff -r e6ce59a07d5f nanojit/LIR.cpp
>--- a/nanojit/LIR.cpp	Mon May 24 09:13:11 2010 -0400
>+++ b/nanojit/LIR.cpp	Mon May 24 10:32:50 2010 -0700
>@@ -806,7 +806,16 @@
>                     return oprnd1;
>                 case LIR_andi:
>                 case LIR_muli:
>+                case LIR_ltui: // unsigned < 0 -> always false
>+#ifdef NANOJIT_64BIT
>+                case LIR_ltuq:
>+#endif
>                     return oprnd2;

This doesn't look right. ltui and ltuq should return insImmI(0), not oprnd2. No?

>+                case LIR_gtui: // unsigned >= 0 -> always true
>+#ifdef NANOJIT_64BIT
>+                case LIR_gtuq: // unsigned >= 0 -> always true
>+#endif
>+                    return insImmI(1);
>                 case LIR_eqi:
>                     if (oprnd1->isop(LIR_ori) &&
>                         oprnd1->oprnd2()->isImmI() &&
>@@ -825,6 +834,13 @@
>                 case LIR_andi:
>                     // x & -1 = x, cmp & 1 = cmp
>                     return oprnd1;
>+                case LIR_gtui:
>+#ifdef NANOJIT_64BIT
>+                case LIR_gtuq:
>+#endif
>+                    // if c == -1, op is u32|u64 > MAX -> always false
>+                    // if c == 1 and oprnd1->isCmp(), op is (0|1) > 1 -> always false
>+                    return insImmI(0);

I think we should split the c == -1 and c== 1 cases. This is very cryptic in the code.

>                 default:
>                     ;
>                 }
(In reply to comment #10)
> This doesn't look right. ltui and ltuq should return insImmI(0), not oprnd2.
> No?

Along that code path, oprnd2 is guaranteed to be LIR_immi (0), so its okay.  Comment could clarify.
(In reply to comment #10)

> I think we should split the c == -1 and c== 1 cases. This is very cryptic in
> the code.

It's cryptic but IMHO acceptable with proper commenting... the alternative is to have two duplicated but identical switch statements, which is not a win. (The cryptic overloading of the switch predates this patch, I'm just taking advantage of it.)
Pinging Andreas for feedback... splitting these cases just adds redundant code, which IMHO is not an improvement; the crypticness can be better addressed via comments in this case. But if you're adamant, I'll split it.
One more ping for feedback from Andreas...
Attachment #447108 - Flags: review?(gal) → review?(nnethercote)
Stealing from Andreas...

(In reply to comment #11)
> > This doesn't look right. ltui and ltuq should return insImmI(0), not oprnd2.
> > No?
> 
> Along that code path, oprnd2 is guaranteed to be LIR_immi (0), so its okay. 
> Comment could clarify.

In which case LIR_ltuq shouldn't be in the switch.  Likewise for LIR_gtuq shortly afterwards.

I also agree about splitting the c=1 and c=-1 cases.  We also have a separate 
'c==1 && v == LIR_muli' case right after.  I'd prefer to have:

  if (c == 0) { ... }
  else if (c == 1) { ... }
  else if (c == -1) { ... }

(with subsequent switches and if-then-elses within the '...' parts).  I think the small amount of switch duplication is worthwhile because it makes it easier to read.

This function should really be split up, it's way too hard to read.  I've filed bug 569753 as a follow-up to do that.
(In reply to comment #15)
> In which case LIR_ltuq shouldn't be in the switch.  Likewise for LIR_gtuq
> shortly afterwards.

Ah, because we're comparing q and i... gotcha. Will remove.
 
> I also agree about splitting the c=1 and c=-1 cases.  We also have a separate 
> 'c==1 && v == LIR_muli' case right after.  

OK, OK, I'm outvoted. Will submit new patch shortly.
Attached patch Patch #4Splinter Review
Revised per nnethercote's suggestions
Attachment #447108 - Attachment is obsolete: true
Attachment #448905 - Flags: review?(edwsmith)
Attachment #447108 - Flags: review?(nnethercote)
Attachment #448905 - Flags: review?(nnethercote)
Attachment #448905 - Flags: review?(nnethercote) → review+
Attachment #448905 - Flags: review?(edwsmith) → review+
Blocks: 560106
pushed to nj-central as 1329:9bdc25ecdb1b
Whiteboard: PACMAN, has-patch → PACMAN, has-patch, fixed-in-nanojit
http://hg.mozilla.org/tracemonkey/rev/9b7c90737b11
Whiteboard: PACMAN, has-patch, fixed-in-nanojit → PACMAN, has-patch, fixed-in-nanojit, fixed-in-tracemonkey
Depends on: 570663
TR:
http://hg.mozilla.org/tamarin-redux/rev/93f5cc536d49
http://hg.mozilla.org/tamarin-redux/rev/ff350031550e
Whiteboard: PACMAN, has-patch, fixed-in-nanojit, fixed-in-tracemonkey → PACMAN, has-patch, fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin
http://hg.mozilla.org/mozilla-central/rev/42fc5e98b20e
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 579348
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.