Detect inferred switches from nested ifs/switches #180

zbanks · 2021-11-27T16:06:26Z

Added a flag --no-switches to disable the new behavior.
Added a test variant to multi-switch that runs w/ --no-switches. The test output matches the original, before these changes.
When implicit switches are detected, there is a comment in the output to help the decomper distinguish them from true jumptable switches.
- Maybe this comment could be removed once we have confidence there are few/no false positives?
IDO is a bit more consistent with what kinds of comparisons it will generate, compared to GCC.
- Behavior is controlled by the --compiler flag.
switch_index values are all screwed up, now that we can both elide or add switches during codegen.
- I've tweaked it so that it never uses the same index twice, but it's possible to have 1 switch have index 0 (which disables comments), and have a second switch have index 1 (so only one switch/labels is commented).
- It's also somewhat common for the output to have noncontinuous switch_indexs.
It looks like GCC may be able to generate some conditions using a pattern like: if (x >= 5) / if (x == 6) / if (x < 6): all 3 of these conditions are true iff x == 5, but the if (x == 5) check is implicit.
- This shows up in functions like PM's func_802403D4_97BA04 (in what would be the temp_v1 == 0xD case). I think it's compiler-generated, but I'm not 100% sure since the fn hasn't been decomp'd.
I don't like that I have to use early_unwrap_ints in if_statements.py. I originally tried to factor out the cast in BinaryOp.scmp(...), but I don't think that's the most robust approach either.
Diffs (although they're quite hard to read/interpret):
- OOT a871499; ~~OOT cf61f36~~
- ~~MM cf61f36~~
- ~~PM cf61f36~~
- Overall, from what I can tell, there are fewer gotos and levels of nesting. I've spot-checked a bunch of decomp'd OOT functions for false positives/negatives.
- The change to is_empty_goto() and its use in build_flowgraph_between() in 1b0c532 introduced a lot of diffs where extra return; statements & /* Duplicate return node #N. Try simplifying control flow for better match */ comments were elided.

simonlindholm

Started reviewing this, still have most of try_build_implicit_switch to go.

flow_graph has a helpful warning which you can remove now:

                    "\n"
                    "(You might need to pass --goto and --no-andor flags as well, "
                    "to get correct control flow for non-jtbl switch jumps.)"

simonlindholm · 2021-11-29T21:34:52Z

src/if_statements.py

@@ -102,7 +104,9 @@ def format(self, fmt: Formatter) -> str:
        body_is_empty = self.body.is_empty()
        if self.index > 0:
            comments.append(f"switch {self.index}")
-        if not self.jump.jump_table:
+        if self.jump.is_implicit:
+            comments.append("implicit")


other suggestions: "complex", "irregular", "branch-based"

src/if_statements.py

simonlindholm · 2021-11-29T23:38:33Z

src/if_statements.py

+    primarily used to manage the overall tree depth.
+    """
+    # The start node must either be an if or a switch
+    if not isinstance(start, (ConditionalNode, SwitchNode)):


if we have a SwitchNode it would also be fine to return and let build_switch_between deal with this, right? since there's no bounds check so we can't have a multi-switch. (no opinion yet on whether it's a good idea)

simonlindholm · 2021-11-29T23:47:05Z

src/if_statements.py

+    # The `default:`-labeled node, if found
+    default_node: Optional[Node] = None
+    # Require at least one ConditionalNode that is not a guard for a SwitchNode
+    has_non_guard_conditional = False


Can't we have a switch that's split into two jump tables and no other conditional nodes?

Hmm, I don't know if the compiler could/would do that? But you might be right...

The intent of this check was to avoid handling the case of just a single SwitchNode & it's ConditionalNode guard, because detecting the default node in this function was less accurate than the existing logic.

I'm going to work on this some more to see if I can make the default node matching better. It would be good to get that working, so that the logic in build_flowgraph_between (elif switch_guard_expr(curr_start) is not None: ...) could be removed in the future?

Apparently IDO doesn't:

void foo(void) { switch (x) { case 0: case 1: case 2: case 3: case 4: case 100: case 101: case 102: case 103: case 104: bar(); } }

results in

00000000 <foo>: 0: 27bdffd8 addiu sp,sp,-40 4: afb00018 sw s0,24(sp) 8: 3c100000 lui s0,0x0 8: R_MIPS_HI16 x c: 8e100000 lw s0,0(s0) c: R_MIPS_LO16 x 10: afbf001c sw ra,28(sp) 14: 2a010005 slti at,s0,5 18: 1420000b bnez at,48 <foo+0x48> 1c: 00000000 nop 20: 260eff9c addiu t6,s0,-100 24: 2dc10005 sltiu at,t6,5 28: 10200012 beqz at,74 <foo+0x74> 2c: 00000000 nop 30: 000e7080 sll t6,t6,0x2 34: 3c010000 lui at,0x0 34: R_MIPS_HI16 .rodata 38: 002e0821 addu at,at,t6 3c: 8c2e0000 lw t6,0(at) 3c: R_MIPS_LO16 .rodata 40: 01c00008 jr t6 44: 00000000 nop 48: 2e010005 sltiu at,s0,5 4c: 10200009 beqz at,74 <foo+0x74> 50: 00000000 nop 54: 00107880 sll t7,s0,0x2 58: 3c010000 lui at,0x0 58: R_MIPS_HI16 .rodata 5c: 002f0821 addu at,at,t7 60: 8c2f0014 lw t7,20(at) 60: R_MIPS_LO16 .rodata 64: 01e00008 jr t7 68: 00000000 nop 6c: 0c000000 jal 0 <foo> 6c: R_MIPS_26 bar 70: 00000000 nop 74: 8fbf001c lw ra,28(sp) 78: 8fb00018 lw s0,24(sp) 7c: 27bd0028 addiu sp,sp,40 80: 03e00008 jr ra 84: 00000000 nop

i.e.

if (x < 5) { if (x < 5) jtbl; } else { if (x - 100 < 5) jtbl; }

which is a bit silly. GCC on the other hand doesn't believe in multiple jump tables: https://decomp.me/scratch/dJz3D

(Just to be clear, the updated code with the irregular_comparison_count variable is more accepting of these types of things, and would accept a pair of jump tables with no other nodes. It doesn't seem to come up in practice, but it doesn't hurt)

simonlindholm · 2021-11-30T11:25:35Z

src/if_statements.py

+    represent jumps to specific case labels, whereas comparisons like `(x < N)` are
+    primarily used to manage the overall tree depth.
+    """
+    # The start node must either be an if or a switch


Suggested change

# The start node must either be an if or a switch

# The start node must either be an if

and you can also remove the start_block_info.switch_control is not None case below

simonlindholm · 2021-11-30T11:43:45Z

src/if_statements.py

+        return None
+
+    # If this new irregular switch uses all of the other switch nodes in the function,
+    # then we no longer need to add labelling comments with the switch_index


(since the change to emit structured switches, are switch indices still actually useful?) Would be nice to clean this up so ordering is consecutive, but it seems like low priority.

There are still a few situations where the case labels get mangled a bit, primarily if the original source uses goto.

simonlindholm · 2021-11-30T11:50:20Z

src/if_statements.py

+                    node_queue.append(node.fallthrough_edge)
+                elif cond.op == "!=" and (
+                    node.block.index > node.conditional_edge.block.index
+                    or context.options.compiler == context.options.CompilerEnum.GCC


do we end up with false positives if we remove this compiler check?

Yeah, an example is this if/else chain from z_vimode in OOT and this if/else chain in z_boss_ganon.

simonlindholm · 2021-11-30T11:52:31Z

src/if_statements.py

+                    # TODO: GCC (maybe?) can emit a series of `<`/`>=` comparisons that limit
+                    # to a specific case. Ex: `x < 9 && x >= 8` is only true for `x == 8`.
+                    # Detecting these would require tracking & checking all of the bounds.
+                    # For now, ignore those, and assume the compiler isn't emitting nonsense.


(if it emits e.g. 8 <= x < 16 to cover case 8..15: I wouldn't call that nonsense)

Ah yeah I should clarify. By "nonsense," I meant something structured like this:

if (x > 10) { if (x == 5) { foo(); } else if (x == 15) { bar(); } } else { if (x == 5) { baz(); } }

This would be equivalent to the following, because foo() is unreachable. But the current logic would not make a switch statement, because it looks like x == 5 is repeated.

switch (x) { case 5: baz(); break; // the `foo();` branch was unreachable case 15: bar(); break; }

simonlindholm · 2021-11-30T11:53:03Z

src/if_statements.py

+                    node_queue.append(node.conditional_edge)
+                elif cond.op == "<" or (
+                    cond.op in (">=", "<=", ">")
+                    and context.options.compiler == context.options.CompilerEnum.GCC


same question

Thanks for checking - it looks like this one isn't needed.
There might be a false-positive in MM's Room_AllocateAndLoad when I remove this check, but the structure is very weird, (removing the other == GCC check doesn't help with the outer if either)

if ((temp_v0_5 != 0) && (temp_v0_5 != -2) && (temp_v0_5 != -7)) { - if ((temp_v0_5 == -8) || (temp_v0_5 == -5) || (temp_v0_5 == -4) || ((temp_v0_5 < 0) && (temp_v0_5 != -1) && (temp_v0_5 != -6))) { - - } else { + switch (temp_v0_5) { /* irregular */ + case -8: + case -5: + case -4: + break; + default: phi_v1_3 = temp_v0_5 - 1; if (temp_v0_5 < 0) { phi_v1_3 = 2; } + break; } phi_a2 = (s32) gSaveContext.respawn[phi_v1_3].roomIndex; } else { phi_a2 = (s32) ((globalCtx + 0x18000)->unk_85C + ((globalCtx + 0x18000)->unk_B48 * 2))->unk_1; }

zbanks added 4 commits November 27, 2021 11:03

WIP: Detect compound switches from nested ifs

76091e5

WIP: Tighten restrictions on detecting switches; add --no-switches

0e99d07

Move cast unwrapping into if_statements

b926569

Comments; tighten cases where implicit switches are detected

4be28ff

zbanks changed the title ~~WIP: Detect compound switches from nested ifs~~ Detect inferred switches from nested ifs/switches Nov 27, 2021

zbanks added 2 commits November 27, 2021 18:28

Remove outdated warning

6b7b274

Comment typos

cf61f36

zbanks marked this pull request as ready for review November 28, 2021 00:04

zbanks requested a review from simonlindholm November 28, 2021 00:04

zbanks added 2 commits November 28, 2021 12:32

Add comments; special-case empty returns to TerminalNode

1b0c532

Require at least one non-guard conditional in implicit switches

a871499

simonlindholm reviewed Nov 29, 2021

View reviewed changes

zbanks added 2 commits November 29, 2021 21:13

Address most of Simon's early PR comments

266c4d3

Clarify has_non_guard_conditional & other comments

da57133

simonlindholm approved these changes Nov 30, 2021

View reviewed changes

zbanks added 2 commits November 30, 2021 11:18

Clarify comments; remove unused checks; PR feedback from Simon

23530f8

Fix --help message/website wording

b1b3137

zbanks merged commit 337bcf6 into matt-kempster:master Nov 30, 2021

zbanks deleted the compound-switches branch November 30, 2021 16:26

zbanks restored the compound-switches branch November 30, 2021 16:26

zbanks deleted the compound-switches branch November 30, 2021 16:26

zbanks restored the compound-switches branch November 30, 2021 16:26

zbanks mentioned this pull request Nov 30, 2021

Fix irregular switch labels so ordering is consecutive #183

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect inferred switches from nested ifs/switches #180

Detect inferred switches from nested ifs/switches #180

zbanks commented Nov 27, 2021 •

edited

Loading

simonlindholm left a comment

simonlindholm Nov 29, 2021

simonlindholm Nov 29, 2021

simonlindholm Nov 29, 2021

zbanks Nov 30, 2021

simonlindholm Nov 30, 2021

zbanks Nov 30, 2021

simonlindholm Nov 30, 2021

simonlindholm Nov 30, 2021

zbanks Nov 30, 2021

simonlindholm Nov 30, 2021

zbanks Nov 30, 2021

simonlindholm Nov 30, 2021

zbanks Nov 30, 2021

simonlindholm Nov 30, 2021

zbanks Nov 30, 2021

	# The start node must either be an if or a switch
	# The start node must either be an if

Detect inferred switches from nested ifs/switches #180

Detect inferred switches from nested ifs/switches #180

Conversation

zbanks commented Nov 27, 2021 • edited Loading

simonlindholm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbanks commented Nov 27, 2021 •

edited

Loading