reactorlabs / rir

GNU General Public License v2.0
62 stars 18 forks source link

Consolidate the feedback origin and type recording #1260

Open fikovnik opened 8 months ago

fikovnik commented 8 months ago

There are a few places in the code that should likely by consolidated:

  1. Updating feedbackOrigin::function without the corresponding index.

Is it the case that the feedbackOrigin::function is used for something without the index? It is not clear, should be documented.

In rir/src/compiler/pir/builder.cpp:131 in Builder::Builder(Continuation* cnt, Value* closureEnv):

        mkenv->updateTypeFeedback().feedbackOrigin.function(
            cnt->owner()->rirFunction());

In rir/src/compiler/pir/builder.cpp:180 in Builder::Builder(ClosureVersion* version, Value* closureEnv):

mkenv->updateTypeFeedback().feedbackOrigin.function(rirFun);
  1. The type recording

It feels that all of those are trying to do something similar. We could pull this into the record_type directly or document why there are differences.

In rir/src/runtime/TypeFeedback.cpp for deopt:

        // FIXME: very similar code is in the recordTypeFeedbackImpl
        // IMHO the one there is more correct. Would it make sense
        // to pull this into the TypeFeedback::record_type()?
        // and get rid of the overload that takes lambda?
        feedback->record_type(origin.idx(), val);
        feedback->record_type(origin.idx(), [&](auto& slot) {
            if (TYPEOF(val) == PROMSXP) {
                if (PRVALUE(val) == R_UnboundValue &&
                    slot.stateBeforeLastForce < ObservedValues::promise)
                    slot.stateBeforeLastForce = ObservedValues::promise;
                else if (slot.stateBeforeLastForce <
                         ObservedValues::evaluatedPromise)
                    slot.stateBeforeLastForce =
                        ObservedValues::evaluatedPromise;
            }
        });

In rir/src/interpreter/interp.cpp in recordForceBehavior:

        c->function()->typeFeedback()->record_type(idx, [&](auto& feedback) {
            if (feedback.stateBeforeLastForce < state) {
                feedback.stateBeforeLastForce = state;
            }
        });

In rir/src/compiler/native/builtins.cpp in recordTypefeedbackImpl:

    feedback->record_type(idx, [&](auto& slot) {
        if (TYPEOF(value) == PROMSXP) {
            if (PRVALUE(value) == R_UnboundValue &&
                slot.stateBeforeLastForce < ObservedValues::promise) {
                slot.stateBeforeLastForce = ObservedValues::promise;
            } else if (slot.stateBeforeLastForce <
                       ObservedValues::evaluatedPromise) {
                slot.stateBeforeLastForce = ObservedValues::evaluatedPromise;
            }
        } else {
            if (slot.stateBeforeLastForce < ObservedValues::value)
                slot.stateBeforeLastForce = ObservedValues::value;
        }
    });