Bug 227171 – Add a new pattern to instruction selector to use EXTR supported by ARM64

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yijia Huang]

...

[Yijia Huang]

This bug will be fixed in the ubfiz patch(bug 227204).

[Yijia Huang]

This is fixed in bug 227204.

[Yijia Huang]

Use this bug report for other purpose: Add ARM64 BFC into AIR opcode

[Yijia Huang]

ARM64 BFC instruction is already added in bug 227202.

Use this bug report for another purpose: Add ARM64 BFXIL

[Yijia Huang]

Created attachment 432567 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/79951628>

[Yijia Huang]

Created attachment 432600 [details]
Patch

[Filip Pizlo]

Comment on attachment 432600 [details]
Patch

Nice!

[Yijia Huang]

(In reply to Filip Pizlo from comment #8)
> Comment on attachment 432600 [details]
> Patch
> 
> Nice!

Thanks for the review.

[Robin Morisset]

Comment on attachment 432600 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=432600&action=review

I like it overall, but I am concerned that you may have broken the instruction selection for sbfiz.

> Source/JavaScriptCore/ChangeLog:13
> +        ### Part A zero extend bitfield ###

I like that, as BitAnd(value, mask) is a lot more canonical throughout the compiler.
But I just checked, and your pattern for sbfiz looks for the ZShr(Shl(value, amount), amount) pattern.
Have you tried running testInsertSignedBitfieldInZero64 on ARM64 ? I would expect it to fail until you replace the pattern for sbfiz in B3LowerToAir.

> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1482
> +                    uint64_t mask = width == datasize ? -1ULL : (1ULL << width) - 1ULL;

nitpick: instead of `-1ULL` on the true side, I would put std::numeric_limits<uint64_t>::max(). It is almost certainly equivalent, but marking a negative number with U(unsigned)LL seems weird to me.

[Saam Barati]

Comment on attachment 432600 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=432600&action=review

> Source/JavaScriptCore/ChangeLog:50
> +        Int @7 = ZShr(@1, @5))

@5 -> @6

> Source/JavaScriptCore/ChangeLog:51
> +        Int @8 = BitOr(@7, @6)

@6 -> @5

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2860
> +                commitInternal(left->child(0));
> +                commitInternal(left);
> +                commitInternal(right);

Why is this valid to do? We never check canBeInternal. Since we're using "is locked" checks, isn't that enough to just grab the temp, but not commit internal?

> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1482
> +                    uint64_t mask = width == datasize ? -1ULL : (1ULL << width) - 1ULL;

can we use numeric_limits<uint64_t>::max instead of "-1ULL"

also, if "width == datasize", can't we just replace ourselves with "value"?

[Yijia Huang]

(In reply to Robin Morisset from comment #10)
> Comment on attachment 432600 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432600&action=review
> 
> I like it overall, but I am concerned that you may have broken the
> instruction selection for sbfiz.
> 
> > Source/JavaScriptCore/ChangeLog:13
> > +        ### Part A zero extend bitfield ###
> 
> I like that, as BitAnd(value, mask) is a lot more canonical throughout the
> compiler.
> But I just checked, and your pattern for sbfiz looks for the ZShr(Shl(value,
> amount), amount) pattern.
> Have you tried running testInsertSignedBitfieldInZero64 on ARM64 ? I would
> expect it to fail until you replace the pattern for sbfiz in B3LowerToAir.

Thanks for the review. That's a good question. The pattern of sbfiz is: 

((src << amount) >> amount) << lsb

where the right shift is signed. And the reduction rule introduced in this patch is an unsigned right shift.

Turn this: ZShr(Shl(value, amount)), amount)
Into this: BitAnd(value, mask)

> > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1482
> > +                    uint64_t mask = width == datasize ? -1ULL : (1ULL << width) - 1ULL;
> 
> nitpick: instead of `-1ULL` on the true side, I would put
> std::numeric_limits<uint64_t>::max(). It is almost certainly equivalent, but
> marking a negative number with U(unsigned)LL seems weird to me.

I'll update all of them. Great point!

[Yijia Huang]

(In reply to Saam Barati from comment #11)
> Comment on attachment 432600 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432600&action=review
> 
> > Source/JavaScriptCore/ChangeLog:50
> > +        Int @7 = ZShr(@1, @5))
> 
> @5 -> @6
> 
> > Source/JavaScriptCore/ChangeLog:51
> > +        Int @8 = BitOr(@7, @6)
> 
> @6 -> @5
> 
> > Source/JavaScriptCore/b3/B3LowerToAir.cpp:2860
> > +                commitInternal(left->child(0));
> > +                commitInternal(left);
> > +                commitInternal(right);
> 
> Why is this valid to do? We never check canBeInternal. Since we're using "is
> locked" checks, isn't that enough to just grab the temp, but not commit
> internal?
> 
> > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1482
> > +                    uint64_t mask = width == datasize ? -1ULL : (1ULL << width) - 1ULL;
> 
> can we use numeric_limits<uint64_t>::max instead of "-1ULL"
> 
> also, if "width == datasize", can't we just replace ourselves with "value"?

Thanks for the review. Working on them.

[Yijia Huang]

Created attachment 432613 [details]
Patch

[Robin Morisset]

Comment on attachment 432613 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=432613&action=review

r=me with a minor comment.
Thank you for the explanation about sbfiz, I had missed that it uses SShr instead of ZShr.
I'll cq+ once all the bots are green (unless you can/want to do it yourself).

> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1491
> +                        replaceWithIdentity(m_value->child(0)->child(0));

I don't think this is needed: in the case where amount1 == 0,the shifts will already be replaced by Identity in handleShiftAmount().

[Yijia Huang]

Created attachment 432621 [details]
Patch

[Yijia Huang]

(In reply to Robin Morisset from comment #15)
> Comment on attachment 432613 [details]
> Patch
> 
> > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1491
> > +                        replaceWithIdentity(m_value->child(0)->child(0));
> 
> I don't think this is needed: in the case where amount1 == 0,the shifts will
> already be replaced by Identity in handleShiftAmount().

Indeed, thanks for pointing it out. Updated.

[Robin Morisset]

Comment on attachment 432621 [details]
Patch

r=me; cq=me

[EWS]

Committed r279470 (239324@main): <https://commits.webkit.org/239324@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 432621 [details].