1446467 - jit: Fuse JmpSrc and JmpDst

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine: JIT, enhancement, P3)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: refactor-jmpsrc.patch

JmpSrc and JmpDst really are the same thing, so there's no reason to have both imo.

Comment 1

[Jan de Mooij [:jandem]]

Comment on attachment 8959609 [details] [diff] [review]
refactor-jmpsrc.patch

Review of attachment 8959609 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense, but one reason for keeping separate classes is for better type safety, although I don't have a strong opinion on this. Nicolas, WDYT?

::: js/src/jit/x86-shared/Patching-x86-shared.h
@@ +58,5 @@
>      int32_t rel = GetInt32(where);
>      return (char*)where + rel;
>  }
>  
> +class JmpOffset {

Maybe even JumpOffset?

@@ +68,3 @@
>        : offset_(offset)
>      {
> +        MOZ_RELEASE_ASSERT(offset == size_t(offset_));

This class is used a lot so I'd prefer MOZ_ASSERT.

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8959609 [details] [diff] [review]
refactor-jmpsrc.patch

Review of attachment 8959609 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/x86-shared/Patching-x86-shared.h
@@ +65,1 @@
>        : offset_(-1)

My take is that this default constructor should be removed, even if it means that nextJump should be rewritten using  Result<JumpOffset, OOM>, and if the jSrc function should duplicate 4 lines of code. This also implies that we should be able to remove all the MOZ_ASSERT which are checking that the offset is not -1, such as in setNextJump and linkJump. And at the end we convert the offset to a size_t.

Then, I like type safety in general. Maybe something like:

  template <int>
  class CodeOffset {
    public:
      explicit CodeOffset(size_t o) : offset_(offset) {}
      size_t offset() const { return offset_; }
    private:
      size_t offset_;
  };
  using JmpSrc = CodeOffset<0>;
  using JmpDst = CodeOffset<1>;

would satisfy both the type safety and code sharing constraints (without as much rewrite).

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Alright, considering the push back here and the amount of work needed for your suggestion, I'll just wontfix this. Feel free to re-open if you want to do it.