IonMonkey: make sure MBasicBlock::loopHeader() returns the loop header of the block.

RESOLVED WORKSFORME

Status

()

Core
JavaScript Engine
RESOLVED WORKSFORME
5 years ago
2 years ago

People

(Reporter: wuwei, Assigned: wuwei)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Currently MBasicBlock::loopHeader() always return null. It might be useful if it returns the correct value.
Created attachment 798725 [details] [diff] [review]
/home/mjrosenb/patches/loophead-r0.patch

Intentionally leaving review field empty for now.  I just ripped this patch out of some larger code that I was working on, and I want to make sure it works by itself.
Attachment #798725 - Flags: review?
Attachment #798725 - Flags: review? → review?(luke)
Comment on attachment 798725 [details] [diff] [review]
/home/mjrosenb/patches/loophead-r0.patch

Luke for the asm.js bits, and jan for the Ion bits.
Attachment #798725 - Flags: review?(jdemooij)
This reminds me of the FindNaturalLoops function we used to have:

http://hg.mozilla.org/integration/mozilla-inbound/file/7faf3ada4920/js/src/ion/IonAnalysis.cpp#l666

It's a bit simpler but maybe less efficient. I'm not sure if it matters though..

If we do want to set the header immediately, it would be good to add something similar to FindNaturalLoops to assert every block has the correct loopHeader() in debug builds. Like AssertGraphCoherency and friends.

Comment 4

5 years ago
Comment on attachment 798725 [details] [diff] [review]
/home/mjrosenb/patches/loophead-r0.patch

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

IIUC, this would add a new graph invariant and it would be a good idea to check that in AssertGraphConsistency or else I surely it'll break and show up only in weird cases.  That got me thinking, though: could this loop-header-setting be merged with the existing dominator analysis?  I haven't thought through the algorithm, but it has a very dominator-y feel to it.

::: js/src/jit/AsmJS.cpp
@@ +2489,5 @@
>              return false;
>          noteBasicBlockPosition(*block, pn);
>          mirGraph().addBlock(*block);
>          (*block)->setLoopDepth(loopDepth);
> +        //(*block)->setLoopHeader(loopHeader);

Did you mean to leave this commented in?

@@ +5413,5 @@
> +
> +        const char *c = AtomToPrintableString(m.cx(), exportedFunc.name(), &bytes);
> +        int x = (int) strdup(c);
> +        masm.ma_mov(Imm32(x), r12);
> +        masm.breakpoint();

This shouldn't be committed (there should be merge conflicts with all this anyhow after bug 909826).

Comment 5

4 years ago
Comment on attachment 798725 [details] [diff] [review]
/home/mjrosenb/patches/loophead-r0.patch

Cancelling for now pending further response.
Attachment #798725 - Flags: review?(luke)
Comment on attachment 798725 [details] [diff] [review]
/home/mjrosenb/patches/loophead-r0.patch

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

Resetting review flag. As I said in comment 3, I think this approach is fine, but it'd be good to add debug checks somewhere near AssertGraphCoherency. Something like this should do the trick I think:

for every block b:
    if b is a loop header:
        assert b->loopheader == b
    else:
        for every predecesor p:
            assert b->loopheader == p->loopheader
        if there's no predecessor:
            assert b->loopheader == null
Attachment #798725 - Flags: review?(jdemooij)
Assignee: general → nobody
(Assignee)

Updated

2 years ago
Assignee: nobody → lazyparser
(Assignee)

Comment 7

2 years ago
Hi Marty, has your patch been landed?
Flags: needinfo?(marty.rosenberg)
(Assignee)

Updated

2 years ago
Flags: needinfo?(marty.rosenberg) → needinfo?(lazyparser)
(Assignee)

Comment 8

2 years ago
so loopHeader() has been removed from MBasicBlock. The class LoopAliasInfo in jit/AliasAnalysis.{h,cpp} has similar function.

WFM.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(lazyparser)
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.