Closed Bug 1007785 Opened 8 years ago Closed 8 years ago

MDefintion::useCount() is confusion

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: sunfish, Assigned: sunfish)

Details

Attachments

(1 file)

Attached patch use-count.patchSplinter Review
RecompileCheck's Codegen checks ins->mir()->useCount(), which is the number of times the SSA value is used as an operand in the MIRGraph. It presumably means to check ins->mir()->script()->getUseCount(), which is the number of times the script has been used.

useCount and defUseCount() are rarely useful; they require a traversal through the linked-list of uses to compute, and most code just needs to know if there are a non-zero numer of uses or if there's exactly one use.

Attached is a patch which fixes RecompileCheck, and also rewrites several uses of useCount() and useDefCount() to use hasUses() and hasDefUses(), adds private DEBUG-only helper functions for counting uses in the few places where it's used, and removes useCount() and defUseCount(), to help prevent future confusion.
Attachment #8419549 - Flags: review?(hv1989)
It was pointed out to me that bug 911738 already has a patch which rewrites the RecompileCheck code and also fixes this. I'd still like to submit this patch to remove useCount(), possibly without the RecompileCheck change removed if desired so that it doesn't conflict with the other patch, to help avoid confusion in the future.
Summary: RecompileCheck uses wrong use count → MDefintion::useCount() is confusion
Comment on attachment 8419549 [details] [diff] [review]
use-count.patch

Review of attachment 8419549 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/CodeGenerator.cpp
@@ +8628,5 @@
>      // Check if usecount is high enough.
>      masm.movePtr(ImmPtr(ins->mir()->script()->addressOfUseCount()), tmp);
>      Address ptr(tmp, 0);
>      masm.add32(Imm32(1), tmp);
> +    masm.branch32(Assembler::BelowOrEqual, ptr, Imm32(ins->mir()->script()->getUseCount()), &done);

MRecompileCheck isn't used yet. This code in here has multiple issues and already gets fixed in bug 911738. It needs ins->mir()->recompileThreshold()
^ is just a quick note. I'll do the review tomorrow normally.
Comment on attachment 8419549 [details] [diff] [review]
use-count.patch

Review of attachment 8419549 [details] [diff] [review]:
-----------------------------------------------------------------

I wouldn't remove

::: js/src/jit/C1Spewer.cpp
@@ +95,5 @@
> +    unsigned count = 0;
> +    for (MUseIterator i(def->usesBegin()); i != def->usesEnd(); i++)
> +        count++;
> +    return count;
> +}

No need for this

::: js/src/jit/CodeGenerator.cpp
@@ +8628,5 @@
>      // Check if usecount is high enough.
>      masm.movePtr(ImmPtr(ins->mir()->script()->addressOfUseCount()), tmp);
>      Address ptr(tmp, 0);
>      masm.add32(Imm32(1), tmp);
> +    masm.branch32(Assembler::BelowOrEqual, ptr, Imm32(ins->mir()->script()->getUseCount()), &done);

So plz remove this adjustment. It will get adjusted in another patch.

::: js/src/jit/IonBuilder.cpp
@@ +1187,5 @@
> +   for (MUseIterator i(def->usesBegin()); i != def->usesEnd(); i++)
> +        if ((*i)->consumer()->isDefinition())
> +            count++;
> +    return count;
> +}

No need for this.

::: js/src/jit/MIR.h
@@ -581,5 @@
> -
> -    // Number of uses of this instruction.
> -    // (only counting MDefinitions, ignoring MResumePoints)
> -    size_t defUseCount() const;
> -

I wouldn't remove these functions here. It can sometimes be handy to have them available during GDB. So I would keep these definitions but put them in "#ifdef DEBUG" and put a comment before: "In most places hasUses() or hasDefUses() should be sufficient. Those are much faster when testing for uses or definition Uses.".

That way we won't have these function in release and people will read read the comment on how they should handle this function...
Attachment #8419549 - Flags: review?(hv1989) → review+
> I wouldn't remove
forward this non finished sentence to /dev/null
Ok, I left the original functions in under DEBUG and added a comment. I had to retain a small change to the RecompileCheck so that it still compiles without DEBUG.

https://hg.mozilla.org/integration/mozilla-inbound/rev/76cfc2848641
(In reply to Dan Gohman [:sunfish] from comment #6)
> Ok, I left the original functions in under DEBUG and added a comment. I had
> to retain a small change to the RecompileCheck so that it still compiles
> without DEBUG.

Yeah makes sense, thanks.
https://hg.mozilla.org/mozilla-central/rev/76cfc2848641
Assignee: nobody → sunfish
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.