Bug 153321 – B3 CSE should be able to match a full redundancy even if none of the matches dominate the value in question

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Patch forthcoming.

This is important for Octane/richards.  And probably other things too.

[Filip Pizlo]

Created attachment 269488 [details]
wrote some code

[Filip Pizlo]

Created attachment 269504 [details]
makes FTL B3 faster than FTL LLVM on Octane/richards

I should probably run more than just richards before r?.

[Geoffrey Garen]

Comment on attachment 269504 [details]
makes FTL B3 faster than FTL LLVM on Octane/richards

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

> Source/JavaScriptCore/dfg/DFGCommon.h:41
> -#define FTL_USES_B3 0
> +#define FTL_USES_B3 1

Revert.

[Filip Pizlo]

(In reply to comment #3)
> Comment on attachment 269504 [details]
> makes FTL B3 faster than FTL LLVM on Octane/richards
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269504&action=review
> 
> > Source/JavaScriptCore/dfg/DFGCommon.h:41
> > -#define FTL_USES_B3 0
> > +#define FTL_USES_B3 1
> 
> Revert.

I will this time!

[Filip Pizlo]

Created attachment 269513 [details]
passing so many tests

[Filip Pizlo]

Created attachment 269523 [details]
current performance standings

[Benjamin Poulain]

Comment on attachment 269513 [details]
passing so many tests

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

I have a question for Load8Z and Load16Z.

> Source/JavaScriptCore/b3/B3EliminateCommonSubexpressions.cpp:64
>  typedef Vector<MemoryValue*, 1> MemoryMatches;

Looks like this can be scoped inside the CSE now.

> Source/JavaScriptCore/b3/B3EliminateCommonSubexpressions.cpp:211
>                      return candidate->offset() == offset
>                          && (candidate->opcode() == Load8Z || candidate->opcode() == Store8);

Say I have 3 blocks: A, B, C.

A:
@1 = Const32(0xffffffff)
@2 = Store8(@1, @mem)
@3 = Jump C.

B: 
@4 = Const32(0xff00ff00)
@5 = Store8(@1, @ mem)
@6 = Jump C.

C:
@7 = Load8Z(@mem)

-> The load-store would nuke the top bits of my constants.

Now after changed into StackSlots and back to SSA, the full values is taken. Am I missing something?

> Source/JavaScriptCore/b3/B3EliminateCommonSubexpressions.cpp:466
>      typedef Vector<Value*, 1> Matches;

Looks like this redefines Matches from B3PureCSE. That's probably unnecessary.

[Filip Pizlo]

(In reply to comment #7)
> Comment on attachment 269513 [details]
> passing so many tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269513&action=review
> 
> I have a question for Load8Z and Load16Z.
> 
> > Source/JavaScriptCore/b3/B3EliminateCommonSubexpressions.cpp:64
> >  typedef Vector<MemoryValue*, 1> MemoryMatches;
> 
> Looks like this can be scoped inside the CSE now.
> 
> > Source/JavaScriptCore/b3/B3EliminateCommonSubexpressions.cpp:211
> >                      return candidate->offset() == offset
> >                          && (candidate->opcode() == Load8Z || candidate->opcode() == Store8);
> 
> Say I have 3 blocks: A, B, C.
> 
> A:
> @1 = Const32(0xffffffff)
> @2 = Store8(@1, @mem)
> @3 = Jump C.
> 
> B: 
> @4 = Const32(0xff00ff00)
> @5 = Store8(@1, @ mem)
> @6 = Jump C.
> 
> C:
> @7 = Load8Z(@mem)
> 
> -> The load-store would nuke the top bits of my constants.
> 
> Now after changed into StackSlots and back to SSA, the full values is taken.
> Am I missing something?

It's a bug!  I'll add a test and fix.

> 
> > Source/JavaScriptCore/b3/B3EliminateCommonSubexpressions.cpp:466
> >      typedef Vector<Value*, 1> Matches;
> 
> Looks like this redefines Matches from B3PureCSE. That's probably
> unnecessary.

[Filip Pizlo]

Created attachment 269537 [details]
the patch

[Benjamin Poulain]

Comment on attachment 269537 [details]
the patch

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

> Source/JavaScriptCore/b3/B3EliminateCommonSubexpressions.cpp:217
> +                        Value* sext = m_proc.add<Value>(

sext->zext?

> Source/JavaScriptCore/b3/B3EliminateCommonSubexpressions.cpp:257
> +                        Value* sext = m_proc.add<Value>(

sext->zext?

> Source/JavaScriptCore/b3/testb3.cpp:9400
> +void testStore8Load8Z(int32_t value)

I think it would be good to have a new test for the new feature too (with the value being stored in predecessors).

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/195434