[webkit-reviews] review granted: [Bug 184165] Add some pointer profiling support to B3 and Air. : [Attachment 336825] proposed patch.

[bugzilla-daemon at webkit.org]

JF Bastien <jfbastien at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 184165: Add some pointer profiling support to B3 and Air.
https://bugs.webkit.org/show_bug.cgi?id=184165

Attachment 336825: proposed patch.

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




--- Comment #3 from JF Bastien <jfbastien at apple.com> ---
Comment on attachment 336825
  --> https://bugs.webkit.org/attachment.cgi?id=336825
proposed patch.

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

r=me with some comments.

> Source/JavaScriptCore/b3/B3LowerMacros.cpp:531
> +				       jumpTable[tableIndex] =
linkBuffer.locationOf(*labels[labelIndex++], tag);

For the two changes above: do we ever generate other jumps to these labels
which need a tag?

Say we have:

switch(i) {
case 1:
oops:
// ...
case 2:
  switch (j) {
    case 4:
    // ...
    case 5:
      goto oops;
  }
case 3:
// ...
}

So you've tagged the inner and outer switch differently, and the dispatch at
the top of each switch has the same tag... What does case 1 and label oops end
up doing? I'm guessing labels are always direct jumps so we're OK? Let's talk
it through tomorrow, I just want to make sure I understand!

> Source/JavaScriptCore/b3/B3LowerMacrosAfterOptimizations.cpp:126
> +		       functionAddress =
m_insertionSet.insert<ConstPtrValue>(m_index, m_origin, tagCFunctionPtr(floorf,
B3CCallPtrTag));

For the 4 above: I think your other idiom with tagCFunctionPtr<...>(f, tag) is
easier to grok that this one.

> Source/JavaScriptCore/b3/testb3.cpp:10236
> +	       root->appendNew<ConstPtrValue>(proc, Origin(),
tagCFunctionPtr<void*>(simpleFunction, B3CCallPtrTag)),

No signature here and below?

> Source/JavaScriptCore/b3/air/AirCCallSpecial.cpp:138
> +	   jit.move(CCallHelpers::TrustedImmPtr(B3CCallPtrTag),
ptrTagRegister);

I thought you used TrustedImm64 for the tags?