Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inlining functions can be optimized to avoid copy-in copy-out when possible #2470

Open
fruffy opened this issue Jul 15, 2020 · 5 comments
Open
Assignees
Labels
enhancement This topic discusses an improvement to existing compiler code. question This is a topic requesting clarification.

Comments

@fruffy
Copy link
Collaborator

fruffy commented Jul 15, 2020

This is something I have observed in an actual large P4 program.
So I have the following snippet:

void inline_dummy(in bit<16> eth_type, out bit<8> dummy_val) {}
control sub_ctrl(in bit<16> eth_type, out bit<8> dummy_val){
    apply {
    }
}

control ingress(inout Headers h, inout Meta m, inout standard_metadata_t sm) {
    sub_ctrl() sub_c;

    apply {
        sub_c.apply(h.eth_hdr.eth_type, h.h.a);
        inline_dummy(h.eth_hdr.eth_type, h.h.b);

    }
}

After the inlining passes (Inline and InlineFunctions), this turns into:

control ingress(inout Headers h, inout Meta m, inout standard_metadata_t sm) {
    apply {
        {
        }
        {
            bit<8> dummy_val_0;
            h.h.b = dummy_val_0;
        }
    }
}

In this case, the semantics of the out parameter for the function call are preserved, but not for the control apply() statement. Which one is correct? Or are both okay? One could argue that not changing the input value at all is a subset of undefined behaviour. Still, this seems inconsistent.

control_nested_inline.p4.txt

@fruffy fruffy changed the title Inlining Controls and out parameters Inlining controls with out parameters Jul 15, 2020
@mihaibudiu
Copy link
Contributor

In either case you get an undefined result.
So this looks correct to me.

@mihaibudiu mihaibudiu added the question This is a topic requesting clarification. label Jul 15, 2020
@jafingerhut
Copy link
Contributor

It definitely seems like this is a case where the compiler is transforming the sub_c.apply() call into a program that is "less undefined" or "fewer possible output states" than the input program, and if one believed that the compiler transformations should somehow preserve undefined behavior, the sub_c.apply() call should also assign an undefined value to h.h.a.

I do not currently have any strong opinions on whether compiler transformations should maximally preserve undefined behavior, or should be allowed to reduce the possible behaviors.

@mihaibudiu
Copy link
Contributor

Perhaps it's a bug, I will investigate further.

@fruffy
Copy link
Collaborator Author

fruffy commented Jul 16, 2020

For what it's worth, I cannot think of a case where this could be harmful. What I found odd was the difference in behaviour between function/action inlining and control inlining.

@mihaibudiu
Copy link
Contributor

The inliner for controls is slightly better optimized: it tries to avoid doing copy-in copy-out when the arguments to not alias or have side-effects. The function inliner always does copy-in copy-out. So I guess this is an optimization opportunity for functions, where we can reuse some of the code for the controls. I will rename this issue to reflect this fact, and mark it as an enhacement. This may help with actions too.

@mihaibudiu mihaibudiu added the enhancement This topic discusses an improvement to existing compiler code. label Jun 29, 2021
@mihaibudiu mihaibudiu changed the title Inlining controls with out parameters Inlining functions can be optimized to avoid copy-in copy-out when possible Jun 29, 2021
@mihaibudiu mihaibudiu self-assigned this Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This topic discusses an improvement to existing compiler code. question This is a topic requesting clarification.
Projects
None yet
Development

No branches or pull requests

3 participants