Investigate optimizer pass to make unaccessed named fields anonymous

[bbannier]

In #1398 we discussed an optimizer pass to make unaccessed named field anonymous. We should have a look at how hard this would be to add, maybe even without adding control flow analysis.

[evantypanski]

I started poking this with a stick, but I wonder if it's still valid.

First, naively implementing this, it worked pretty well. There is one caveat, that printing will be a bit unexpected: you'll print the unit, but now it's transient and your field randomly disappeared. So some statements on self will need to be treated as using each field.

But, more importantly - I think custom host applications make it so we don't know anything about whether or not a field is accessed. If it's a field within a public unit, then it can be accessed. Since this should (?) be basically all units, I don't think this change will actually do anything in real parsers. We can't tell if it's accessed from a public unit from a custom host application... I think :)

Just naively, it changes how this program behaves:

module Test;

public function f(d: Inner) : vector<uint8> {
    return d.this_is_used;
}

public type Data = unit {
    inner: Inner;

    on %done {
        print self;
    }
};

type Inner = unit {
    this_is_unused: uint8[5];
    this_is_used: uint8[5];

    on %done {
        f(self);
    }
};

Making it transient:

$ echo "12345abcde" | spicy-driver ../test.spicy
[$inner=[$this_is_unused=[], $this_is_used=[97, 98, 99, 100, 101]]]

Current main:

$ echo "12345abcde" | spicy-driver ../test.spicy
[$inner=[$this_is_unused=[49, 50, 51, 52, 53], $this_is_used=[97, 98, 99, 100, 101]]]

so maybe this shouldn't be done - or am I missing something? At best it seems like it won't actually do anything important.

[rsmmr]

custom host applications make it so we don't know anything about whether or not a field is accessed.

Yeah, I've been wondering about this more generally: with a custom host application, basically any optimization changing something about the public parts of the generated C++ code, is problematic. At the same time, in practice this hardly matters because (1) we know it's not an issue for Zeek; and (2) beyond Zeek, there isn't really any serious other host application out there anyways (afaict).

What would you guys think about adding a new compiler flag to enable "agressive optimizations" where the Spicy compiler becomes free to optimize anything inside a single HLTO without regards to any specifics of the generated C++ code. Basically, the generated code would become a black box and the only safe way to interact with it would be runtime inspection of what's available in terms of parsers and types (and types may look different than in the Spicy source code). Zeek is already doing it this way, and we'd just generally set that flag for any Zeek analyzer.

There is one caveat, that printing will be a bit unexpected: you'll print the unit, but now it's transient and your field randomly disappeared.

Yes, that's a case that wouldn't be solved by that "agressive" flag: as you say, in some sense the printing is an access to the field, and optimizing it out is a semantic change. However, the main impact is that the output becomes confusing, because it looks like something's broken. I'm wondering if we could make this render like this instead:

[$inner=[$this_is_unused=<optimized out>, $this_is_used=[97, 98, 99, 100, 101]]]

Then it would be obvious what's going on.

[evantypanski]

What would you guys think about adding a new compiler flag to enable "agressive optimizations"

Maybe "aggressive optimizations" can just become new policy after a certain release? I feel like I've walked back on my own opinion here. I don't really mind Spicy acting more as a toolchain that will try to aggressively optimize by default, then you have to explicitly do things to stop that. So in this case, we can document a way to "use" the fields and just say if you want them, use them (like a _ = self.this_is_unused kind of thing).

I like this since it adds more friction to the slow case, while making the fast case easy. Part of me wonders if we add another flag, then the "less aggressive optimizations" flag would just be unused in practice.

Either way, it seems good to have a way to do more potentially breaking optimizations, since that's where the real speedup that the C++ compiler can't provide will come from. At least from a codegen standpoint

[$inner=[$this_is_unused=<optimized out>, $this_is_used=[97, 98, 99, 100, 101]]]

Yes, that looks really nice, I like that!

[rsmmr]

So in this case, we can document a way to "use" the fields and just say if you want them, use them (like a _ = self.this_is_unused kind of thing).

The nice thing about an option is that it would make semantics very clear, including for further optimizations that we may be adding over time: either people can rely on the generated C++ code or they can't. Documenting ways to get around certain individual optimizations feels brittle in contrast, and also hard to explain.

Part of me wonders if we add another flag, then the "less aggressive optimizations" flag would just be unused in practice.

Yeah, I see that risk, although it's probably pretty well testable at least to ensure we don't break it: when a new optimization comes around that can change the C++ interface, it should be easy to add tests for both cases.

There is a general question if we want to keep the "use-the-C++-code-directly" use case going forward; there's clearly not much demand for it so far. But I'm not quite ready to let that go.

[bbannier]

There is a general question if we want to keep the "use-the-C++-code-directly" use case going forward; there's clearly not much demand for it so far. But I'm not quite ready to let that go.

Me and Evan chatted about this a while back as well in the context of how we rely on public to mean that the C++ interface needs to behave as exactly as specified in Spicy. This already today has the downside that we probably apply way fewer cleanups on public unit than possible to account for a very small number users interfacing with C++.

One way around this and forward could be that in that "aggressive" mode we force users to declare at least types they rely on externally, but we could extend that to fields as well. This could look similar to how users export types in Zeek EVT files.