[webkit-reviews] review requested: [Bug 170215] WebAssembly: Air::Inst::generate crashes on large binary on A64 : [Attachment 309022] patch

[bugzilla-daemon at webkit.org]

JF Bastien <jfbastien at apple.com> has asked  for review:
Bug 170215: WebAssembly: Air::Inst::generate crashes on large binary on A64
https://bugs.webkit.org/show_bug.cgi?id=170215

Attachment 309022: patch

https://bugs.webkit.org/attachment.cgi?id=309022&action=review




--- Comment #27 from JF Bastien <jfbastien at apple.com> ---
Created attachment 309022

  --> https://bugs.webkit.org/attachment.cgi?id=309022&action=review

patch

> > Source/JavaScriptCore/ChangeLog:21
> > +	     We now unconditionally pin the dataTempRegister on ARM64, and use
> > +	     it when immediates don't fit.
> 
> Have you measured perf on WasmBench?

WasmBench crashes without my patch :(
Works fine with my patch... but the numbers are meaningless on their own!

TitzerBench is perf neutral before / after.


> > Source/JavaScriptCore/ChangeLog:38
> > +	     Number 2. is more complex: not all Inst can receive a stack
> > +	     argument whose base register isn't SP or FP. Specifically,
> > +	     Patchpoints and Stackmaps get very sad because they just want to
> > +	     know the offset value, but when we materialize the offset as
> > +	     follows:
> > +
> > +		 Move (spill337), (spill201), %r0, @8735
> > +
> > +	     Becomes (where %r16 is dataTempRegister):
> > +		 Move $1404, %r16, @8736
> > +		 Add64 %sp, %r16, @8736
> > +		 Move (%r16), 2032(%sp), %r0, @8736
> > +
> 
> This example confuses me. Why can't we encoded 1404 but we can encode 2032?

Because of isValidScaledUImm12, this clause: offset & ((datasize / 8) - 1)


> > Source/JavaScriptCore/ChangeLog:44
> > +	     generate them, otherwise we generate Addr as shown above.
> 
> What if they expect the base of the address to be SP/FP and they don't
> accept ExtendedOffsetAddr?

Then it shouldn't be a ExtendedOffsetAddr? I'm not sure I understand.


> > Source/JavaScriptCore/b3/B3CheckSpecial.cpp:147
> > +	 return admitsStackImpl(numB3Args(inst), m_numCheckArgs + 1, inst,
argIndex);
> 
> Why admitsStackImpl here? Maybe  the function needs a new name?

My mental model may be wrong, but my thinking was: if it admits a stack value
then it could maybe admit an extended address. This case first disallows
extended address for hidden branches, but lets others through as admitsStack
would.

Maybe I should just call admitsStack here?


> > Source/JavaScriptCore/b3/B3PatchpointSpecial.cpp:136
> > +bool PatchpointSpecial::admitsExtendedOffsetAddr(Inst&, unsigned)
> > +{
> > +	 return true;
> > +}
> 
> Why is this just true, but admitsStack above is not unconditional? Seems
> wrong to me, especially given the above checks if things need registers, etc.

Right, you're expressing my mental model above... My understanding was that I
could relax it here because Patchpoint won't generate an Addr for a register.
But re-reading the code that seems wrong because admitsStackImpl on Void origin
does more than this.

So I'll make it just call admitsStack.

(In reply to Saam Barati from comment #26)
> Also, can you please add some tests for this?
> 
> It seems like you could trigger this code inside testb3 either with some
> carefully crafted code, or perhaps just using O0.

The code does trigger, even on x86! It's not specific to huge frames on
purpose, so it's not a corner case. Extended address is there super often.


(In reply to Saam Barati from comment #24)
> (In reply to JF Bastien from comment #23)
> > Created attachment 308890 [details]
> > patch
> > 
> > > > Source/JavaScriptCore/b3/B3Common.cpp:76
> > > > +    return static_cast<GPRReg>(+MacroAssembler::dataTempRegister);
> > > 
> > > Why the +?
> > 
> > You found the 💎 gem 💎 hidden in this patch! 🎉
> > 
> > dataTempRegister in the header is actually a *declaration* and not a
> > *definition*. Even adding constexpr (as I did) doesn't make it a
definition.
> > This causes the linker to crap itself on missing dataTempRegister  symbol.
> > Sad indeed! Why does it work elsewhere? ✨ The magic of C++ ✨ One fix is to
> > switch to C++17 and use inline variables, another is to just declare
> > dataTempRegister in the header and then add a definition in the .cpp file
> > which is super ugly and **is deprecated in C++17** in favor of inline
> > variables. But the other fix... the beautiful fix... is to turn the use of
> > dataTempRegister into an rvalue! This beautiful usage of unary plus
> > magically makes the problem go away because now we're not asking the linker
> > to get this value from oh-so-far away it would surely need a relocation!
No,
> > we're just rvalue-ing, nothing to see here linker. So there you have it,
> > unary plus isn't totally useless. It should be useless, but here it's
useful
> > in compensating for something ridiculous. 
> I don't get why constexpr wouldn't fix this.
> What about a "using" declaration?

See last page of http://wg21.link/P0386R2
It's a declaration, not a definition, until C++17.