Closed Bug 807824 Opened 7 years ago Closed 7 years ago

IonMonkey: don't clobber output reg in oolcall

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: h4writer, Assigned: h4writer)

Details

Attachments

(1 file, 2 obsolete files)

Currently we save the live RegisterSet before calling the oolcall. Afterwards we restore all registers. We also restore register where the result of the call is located. As a result we loose the information in the register. I.e. the result of the oolcall
Assignee: general → hv1989
Attachment #677581 - Flags: review?(nicolas.b.pierron)
This sounds like a bug in the register allocator? It should not be saving the output register in the live register set. Do you have a test case?
Comment on attachment 677581 [details] [diff] [review]
Don't override result register in OOL call

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

If you want to go that way, you should keep the register in the saveLive set, because it has to be marked, and conditionally restore it in PopRegsInMask.

::: js/src/ion/shared/CodeGenerator-shared-inl.h
@@ +183,5 @@
> +    JS_ASSERT(!ins->isCall());
> +    LSafepoint *safepoint = ins->safepoint();
> +    RegisterSet set = safepoint->liveRegs();
> +    set = set.Not(except);
> +    masm.PushRegsInMask(set);

The number of register push here does not correspond to the number of register expected when we are marking the live registers pushed before the exit frame.

see MarkIonJSFrame, all usage of the SafepointReader.
Attachment #677581 - Flags: review?(nicolas.b.pierron)
Attached patch Conditionally restore registers (obsolete) — Splinter Review
Like requested ;)
Attachment #677581 - Attachment is obsolete: true
Attachment #677613 - Flags: review?(nicolas.b.pierron)
Comment on attachment 677613 [details] [diff] [review]
Conditionally restore registers

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

Sounds good, handle the StoreValueTo<T>(T) case by delegating the query to the corresponding register set.

::: js/src/ion/IonMacroAssembler.h
@@ +257,5 @@
>      void PushRegsInMask(RegisterSet set);
>      void PushRegsInMask(GeneralRegisterSet set) {
>          PushRegsInMask(RegisterSet(set, FloatRegisterSet()));
>      }
>      void PopRegsInMask(RegisterSet set);

nit:

void PopRegsInMask(RegisterSet set) {
  PopRegsInMaskIgnore(set, RegisterSet());
}

::: js/src/ion/shared/CodeGenerator-shared.h
@@ +468,5 @@
>      inline void generate(CodeGeneratorShared *codegen) const {
>          codegen->storeResultValueTo(out_);
>      }
> +    inline RegisterSet clobbered() const {
> +        return RegisterSet(); // No register gets clobbered

nit: This Output can be a ValueOperand, or a TypedOrValueOperand or anything else.  I will recommend you to add a RegisterSet function to each of these types and to get the clobbered set out of them.
Attachment #677613 - Flags: review?(nicolas.b.pierron)
I couldn't have been awake, forgetting to add these registers.
Attachment #677613 - Attachment is obsolete: true
Attachment #677808 - Flags: review?(nicolas.b.pierron)
Comment on attachment 677808 [details] [diff] [review]
Conditionally restore registers

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

Sounds good.
Attachment #677808 - Flags: review?(nicolas.b.pierron) → review+
https://hg.mozilla.org/mozilla-central/rev/516370bc10b7
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.