Closed Bug 1433959 Opened 2 years ago Closed 2 years ago

Convert GetIntrinsic inline cache to CacheIR

Categories

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

All
Unspecified
enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: mgaudet, Assigned: mgaudet)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

No description provided.
Initial patch to convert GetIntrinsic to CacheIR.

Specific notes:

- The attachment site doesn't end in |return GetIntrinsicOperation|. This
  is required in this design (I believe) because we're baking the result of
  that call into the IC.
- I've hand-checked the CacheIR log results, however, this doesn't have a
  stand-alone test case. As near as I can tell, the majority of the test cases
  we have already cover the code quite well.
Assignee: nobody → mgaudet
Status: NEW → ASSIGNED
Attachment #8946312 - Flags: review?(jdemooij)
Attachment #8946312 - Attachment is obsolete: true
Attachment #8946312 - Flags: review?(jdemooij)
Attachment #8946446 - Flags: review?(jdemooij)
(I would totally appreciate any insight on any better testing for this one BTW)
Comment on attachment 8946446 [details] [diff] [review]
Convert GetIntrinsic inline cache to CacheIR

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

Forwarding to Ted to spread the reviews/knowledge a bit.
Attachment #8946446 - Flags: review?(jdemooij) → review?(tcampbell)
Comment on attachment 8946446 [details] [diff] [review]
Convert GetIntrinsic inline cache to CacheIR

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

Generally looks good. Let's not put in those Ion implementations of load value until we have a real use case. Ion bakes in the constants directly, so there wont be an Ion variant of this IC.

::: js/src/jit/CacheIR.h
@@ +1572,5 @@
>  };
>  
> +class MOZ_RAII GetIntrinsicIRGenerator : public IRGenerator
> +{
> +

Nit: remove empty line

@@ +1580,5 @@
> +    void trackNotAttached();
> +
> +  public:
> +    GetIntrinsicIRGenerator(JSContext* cx, HandleScript, jsbytecode* pc, ICState::Mode,
> +                            HandleValue val); //MG:XXX: Should this be HandleValue, GCPtrValue?

Values flowing through call arguments are generally always HandleValue (which derive from some Rooted<T> higher up the stack. GCPtr is for storage in memory. The original IC described the in-memory object and used GCPtr, while for CacheIR that is hidden from you and handled by CacheIRWriter::addStubField.

::: js/src/jit/IonCacheIRCompiler.cpp
@@ +853,5 @@
>  }
>  
>  bool
> +IonCacheIRCompiler::emitLoadValueResult()
> +{

Let's use  |MOZ_CRASH("Baseline-specific op");| instead of giving an implementation that we can't test.
Attachment #8946446 - Flags: review?(tcampbell)
Attachment #8946446 - Attachment is obsolete: true
Comment on attachment 8948184 [details] [diff] [review]
Convert GetIntrinsic inline cache to CacheIR

Nits addressed

Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93d41c44ad736a416fbd9b697882450ce40e0207
Attachment #8948184 - Flags: review?(tcampbell)
Comment on attachment 8948184 [details] [diff] [review]
Convert GetIntrinsic inline cache to CacheIR

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

Looks good. Just remove the |valueStubFieldPtr| dead code.

::: js/src/jit/IonCacheIRCompiler.cpp
@@ +127,4 @@
>          return (T)readStubInt64(offset, StubField::Type::RawInt64);
>      }
>  
> +    Value* valueStubFieldPtr(uint32_t offset) {

This is now dead, so please remove.

(Also, the reason that |expandoGenerationStubFieldPtr| below looks funny is because the field is not baked-in for Ion and actually can be updated in the stub. This wouldn't have been what we wanted for GETINTRINSIC anyways)
Attachment #8948184 - Flags: review?(tcampbell) → review+
Status: ASSIGNED → NEW
Priority: -- → P3
Comment on attachment 8948716 [details] [diff] [review]
Convert GetIntrinsic inline cache to CacheIR

Carrying tcampbell's r+, after addressing nit. Rebased patch on central. 

Was a mildly hairy rebase, so here's a new try build: 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c29a258fddad43a2bb640323bd82327832dd101b
Attachment #8948716 - Flags: review+
Still looks good to me.
Attachment #8948184 - Attachment is obsolete: true
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a643f7b63c93
Convert GetIntrinsic inline cache to CacheIR. r=tcampbell
Keywords: checkin-needed
Backed out changeset a643f7b63c93 (bug 1433959) for build bustage

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/65aecc4ffa4a2b14defcd29cf7608218d119cee0

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a643f7b63c9372f7062d62aa509373c77ace8592

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=160643212&repo=mozilla-inbound&lineNumber=7283

[task 2018-02-06T18:50:51.283Z] 18:50:51     INFO -  In file included from /builds/worker/workspace/build/src/js/src/jit/BaselineIC.cpp:20:
[task 2018-02-06T18:50:51.283Z] 18:50:51     INFO -  In file included from /builds/worker/workspace/build/src/js/src/jit/BaselineCacheIRCompiler.h:11:
[task 2018-02-06T18:50:51.283Z] 18:50:51     INFO -  /builds/worker/workspace/build/src/js/src/jit/CacheIR.h:1039:32: error: Type 'JS::Value' must not be used as parameter
[task 2018-02-06T18:50:51.284Z] 18:50:51     INFO -      void loadValueResult(Value val) {
[task 2018-02-06T18:50:51.284Z] 18:50:51     INFO -                                 ^
[task 2018-02-06T18:50:51.284Z] 18:50:51     INFO -  /builds/worker/workspace/build/src/js/src/jit/CacheIR.h:1039:32: note: Please consider passing a const reference instead
[task 2018-02-06T18:50:51.284Z] 18:50:51     INFO -  1 error generated.
[task 2018-02-06T18:50:51.284Z] 18:50:51     INFO -  /builds/worker/workspace/build/src/config/rules.mk:1047: recipe for target 'Unified_cpp_js_src11.o' failed
[task 2018-02-06T18:50:51.285Z] 18:50:51     INFO -  make[4]: *** [Unified_cpp_js_src11.o] Error 1
[task 2018-02-06T18:50:51.285Z] 18:50:51     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/js/src'
[task 2018-02-06T18:50:51.285Z] 18:50:51     INFO -  make[4]: *** Waiting for unfinished jobs....
[task 2018-02-06T18:50:51.285Z] 18:50:51     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/config/external/icu/common'
Flags: needinfo?(mgaudet)
Attachment #8948716 - Attachment is obsolete: true
Comment on attachment 8948758 [details] [diff] [review]
Convert GetIntrinsic inline cache to CacheIR

Addressed static analysis failure that caused the patch to need to be reverted: 

Issue was passing a complete JS::Value, rather than a |const JS::Value&|. 

Verifying change w/ this try build:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d679c92caeb21cd7bf6d9a6b4a3ab4aa60625fe
Flags: needinfo?(mgaudet)
Attachment #8948758 - Flags: review?(tcampbell)
Comment on attachment 8948758 [details] [diff] [review]
Convert GetIntrinsic inline cache to CacheIR

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

Interesting... That looks like the right fix.
Attachment #8948758 - Flags: review?(tcampbell) → review+
Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/96333b8e8e25
Convert GetIntrinsic inline cache to CacheIR r=tcampbell
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/96333b8e8e25
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.