[Webkit-unassigned] [Bug 198604] Refactoring of architectural Register Information

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jun 12 02:10:43 PDT 2019


https://bugs.webkit.org/show_bug.cgi?id=198604

--- Comment #22 from Paulo Matos <pmatos at igalia.com> ---
(In reply to Caio Lima from comment #18)
> Comment on attachment 371733 [details]
> Revised Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=371733&action=review
> 
> I didn't read everything, but I've left some comments.
> 

Thanks for the comments.

> > Source/JavaScriptCore/assembler/ARM64Assembler.h:169
> > +//   in ARM64Registers.h on the macro definition of REGNS 
> 
> nit: There is some extra space here.
> 

ok

> > Source/JavaScriptCore/assembler/ARM64Assembler.h:173
> > +#define REGISTER_ID(id, nsid, name, r, cs) id,
> 
> nit: Since we are not using `r` and `cs`, I think we could remove them.
> 
> > Source/JavaScriptCore/assembler/ARM64Assembler.h:185
> > +#define REGISTER_ID(id, nsid, name) id,
> 
> ditto.
> 
> > Source/JavaScriptCore/assembler/ARM64Assembler.h:194
> > +#define REGISTER_ID(id, nsid, name, r, cs) id,
> 
> Ditto.
> 
> > Source/JavaScriptCore/assembler/ARM64Assembler.h:229
> > +#define REGISTER_NAME(id, nsid, name, r, cs) name,
> 
> Ditto.
> 
> > Source/JavaScriptCore/assembler/ARM64Assembler.h:251
> > +#define REGISTER_NAME(id, nsid, name, r, cs) name,
> 
> Ditto.
> 

I cannot remove the unused arguments. They are used by some architecture at some point. To make this truly generic, you need to put the arguments, otherwise RegisterSet.cpp won't compile. It uses these arguments to determine the calleesaved and reserved registers.

> > Source/JavaScriptCore/assembler/ARM64Registers.h:29
> > +#define REGNS ARM64Registers
> 
> One suggestion is to use `REGISTER_NAMESPACE` instead of `REGNS`.
> 

I am happy with verbose! :) I actually had done that changed elsewhere when privately shared your initial comments but forgot to change it here.

> > Source/JavaScriptCore/assembler/ARM64Registers.h:36
> > +#define macro(m, ...) m(__VA_ARGS__)
> 
> Over JSC codebase we usually have `FOR_EACH...(macro)` style. Using `macro`
> seems a little bit confusing to me. Can we rename it to `macro_forward` or
> something else?

ok

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190612/7c5b8945/attachment.html>


More information about the webkit-unassigned mailing list