...
This bug will be fixed in the ubfiz patch(bug 227204).
This is fixed in bug 227204.
Use this bug report for other purpose: Add ARM64 BFC into AIR opcode
ARM64 BFC instruction is already added in bug 227202. Use this bug report for another purpose: Add ARM64 BFXIL
Created attachment 432567 [details] Patch
<rdar://problem/79951628>
Created attachment 432600 [details] Patch
Comment on attachment 432600 [details] Patch Nice!
(In reply to Filip Pizlo from comment #8) > Comment on attachment 432600 [details] > Patch > > Nice! Thanks for the review.
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.
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"?
(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!
(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.
Created attachment 432613 [details] Patch
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().
Created attachment 432621 [details] Patch
(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.
Comment on attachment 432621 [details] Patch r=me; cq=me
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].