[webkit-reviews] review denied: [Bug 228047] Add Pre/Post-Indexed Address Mode to Air for ARM64 : [Attachment 434573] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Yijia Huang <yijia_huang at apple.com>'s
request for review:
Bug 228047: Add Pre/Post-Indexed Address Mode to Air for ARM64
https://bugs.webkit.org/show_bug.cgi?id=228047

Attachment 434573: Patch

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




--- Comment #29 from Filip Pizlo <fpizlo at apple.com> ---
Comment on attachment 434573
  --> https://bugs.webkit.org/attachment.cgi?id=434573
Patch

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

I think you have a bug in how your new Arg works - it says that it just Uses
its input.

Also, you should factor your phase out into its own file.

> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:3165
> +bool canonicalize(Procedure& proc)
> +{
> +    PhaseScope phaseScope(proc, "canonicalize");
> +    ReduceStrength reduceStrength(proc);
> +    return reduceStrength.canonicalizeIncrementAddress();
> +}

I think you need a more descriptive name at the B3 level namespace than
"canonicalize".  I would call it something like "canonicalizePrePostIncrements"
and maybe I would even conditionalize it so it only runs on ARM64.

Also, I wouldn't make this a method of ReduceStrength.	It doesn't seem like it
needs to be.  Seems like this should be a standalone function, in its own file.
 Like "B3CanonicalizePrePostIncrements.h" and
"B3CanonicalizePrePostIncrements.cpp".

> Source/JavaScriptCore/b3/air/AirArg.h:1395
>	       functor(m_base, Use, GP, argRole == UseAddr ? argWidth :
pointerWidth());

This is wrong now - it'll claim that pre/post increment "uses" are just Uses. 
They are UseDef.