Remove redundant comparator inputs

[KonaeAkira]

Whenever a node has links to both the default input and the side input of a comparator, one of those links is often completely irrelevant because it gets cancelled out by the other. This PR adds a new optimization pass to remove those redundant links.

IRIS Mandelbrot benchmark:

master:
DONE in 37.298039635s processing 135000000 ticks, effective rtps: 3619493.177687487
DONE in 38.017733937s processing 135000000 ticks, effective rtps: 3550974.4011495104
DONE in 37.464473919s processing 135000000 ticks, effective rtps: 3603413.7378220367
DONE in 37.449590958s processing 135000000 ticks, effective rtps: 3604845.7819313304

remove-redundant-comparator-inputs:
DONE in 37.079987871s processing 135000000 ticks, effective rtps: 3640777.8899405347
DONE in 37.288002602s processing 135000000 ticks, effective rtps: 3620467.4581512464
DONE in 37.474739122s processing 135000000 ticks, effective rtps: 3602426.678955761
DONE in 37.189786422s processing 135000000 ticks, effective rtps: 3630028.913533619

[KonaeAkira]

The pattern is commonly seen in comparator-based XOR gates:

[Sloimayyy]

I think the correct implementation of this optimization would be to fuse both inputs together into one far input. There are situations where a comparator gets both inputs from a single one where it outputs something that is different. Example:

C R
R R
L

C = Comparator on subtract mode
R = Redstone dust
L = Lever

The comparator will actually output 15 - 13 = 2 when the lever is pressed, but if I understand your implementation correctly the comparator would output 15.
If I understood your code, the fix would be to instead replace both inputs into the comparator into a single back input that's 13 blocks away.

Though I don't know how viable removing/replacing comparator input links is, because this could disrupt some sub-tick timings MCHPRS does simulate, but ultimately I don't know the codebase enough to be certain.

[KonaeAkira]

Looks like I didn't think things through. A comparator on subtract mode does subtraction and not just comparison 🤦.

Fusing the inputs into 15 - 13 = 2 doesn't work if the comparator has other inputs, so in case of subtract mode, we'll also need to check if the links we're fusing are the only links.

I'm putting the PR back to draft until I can fix it after work.

[BramOtte]

Have you considered how far-inputs might effect this?
If you don't know, when a comparator is reading a container through a block it will overwrite its default input with the value of the container unless the default input is 15 then it will be 15.
I also implemented something similar to this tho sadly I didn't take these into account either. (https://github.com/BramOtte/MCHPRS/blob/4d8c22d1d2755a84958c02c819ea9742c6defa5c/crates/redpiler/src/passes/cancelling_comparator_edges.rs)

[BramOtte]

I think the correct implementation of this optimization would be to fuse both inputs together into one far input. There are situations where a comparator gets both inputs from a single one where it outputs something that is different. Example:

C R R R L

C = Comparator on subtract mode R = Redstone dust L = Lever

The comparator will actually output 15 - 13 = 2 when the lever is pressed, but if I understand your implementation correctly the comparator would output 15. If I understood your code, the fix would be to instead replace both inputs into the comparator into a single back input that's 13 blocks away.

Though I don't know how viable removing/replacing comparator input links is, because this could disrupt some sub-tick timings MCHPRS does simulate, but ultimately I don't know the codebase enough to be certain.

I think currently the order with which each output link is evaluated is not accurate to vanilla at all anyway since we do not take into account locationallity fully and the direct backend also reorders output links to improve performance, not to mention some passes also remove entire nodes which would cause similar issues.
Now we could argue that MCHPRS shouldn't be messing with the update order in this way, but removing these redundant links does not make MCHPRS significantly less accurate than it already is.
I think if you want to keep things 100% accurate optimizations become quite difficult in general.

[BramOtte]

I also compared this with my implementation, this PR seems to remove the same amount of edges, but this does it seem slightly slower (9.6..9.9ms vs 5..5.6ms).
The cost difference is not really a big issue if this approach has other benefits tho since the other passes take significantly longer.
Does this cover any extra cases that are perhaps not in iris? I know my code has one case which isn't in iris.
We should probably add tests for the different cases.

[KonaeAkira]

It looks like our implementations optimize the same cases. The only difference I can spot is that you don't handle duplicate edges, i.e. multiple edges from the same node to the same comparator input side, but that shouldn't matter as long as the new pass runs after the edge dedup pass.

It also wasn't my goal to maximize the performance of the optimization pass. The performance just has to be "good enough".

[KonaeAkira]

On another topic, ideally, #216 gets merged before this PR so I can add tests for the new optimization pass.