Do not code fold ifs with concrete arms

[tlively]

Code folding does not support folding tails that produce concrete
values, but it previously did not check for this condition when deciding
whether to attempt to code fold ifs. As a result, code folding would
proceed on ifs with concretely typed arms. The incorrect block types
produced by the folding logic as a result of the violated assumption
that the folded tails would never produce concrete values were papered
over by later refinalization, so this never caused problems.

However, an upcoming change (#7094) that relaxes the typing of ifs to
allow them to be unreachable whenever their conditions are unreachable
makes it possible for the violated assumptions in code folding to cause
problems that are not fixed by refinalization. Fix code folding to
disallow folding of concretely typed if arms and add a test that would
fail once #7094 lands without this fix.

[kripken]

We don't support that for brs, but for ifs I think we can? New line 259 has the logic to keep the concrete type the same.

[tlively]

The comment on lines 502 to 508 explains one of the places that assumes we never have to handle folding concrete types.

The comment // we must ensure we present the same type as the if had on line 258 is also wrong; if an if is unreachable because of an unreachable condition, the ifTrue arm might have some other type.

[kripken]

The comment on line 502 looks correct to me, but it relates to blocks and brs, and not ifs, unless I'm confused?

Good point about unreachability, it is possible we aren't handling that well enough here. But I think we can add that without removing this optimization.

But with that said, I see we have that merging of arms logic in OptimizeInstructions:

binaryen/src/passes/OptimizeInstructions.cpp

Line 1140 in 485ac7d

// The sides are identical, so fold. If we can replace the If with one

So this looks like a redundant optimization actually, and perhaps we can remove it from here. But it is possible we don't handle unreachability fully there, so that might need fixing.

[tlively]

The merging of if arms is handled by the same code that optimizes blocks and brs, so it’s not really possible that we support concrete types for one and not the others. It just happened to work out by accident until now because of the extra refinalization.

I’ll look into whether we can remove the optimizations of Ifs here completely or support concrete types for all folded branches.

[tlively]

I don't think the folding in OptimizeInstructions can completely replace the optimizations here because it only works if the arms are completely identical, whereas this pass only requires identical suffixes.

[kripken]

Oh, it looks like line 251 compares the entire ifTrue and ifFalse arms, while 263+ looks at suffixes. Was your change needed for one of the two?

[tlively]

The test introduced here exposed bugs in the suffix folding code (in the presence of relaxed unreachable ifs), but #7094 also has to fix a bug with the folding of equivalent arms, so both kinds of If optimizations here are broken in the presence of concrete arms.

[kripken]

The new test below has unreachability, and I agree we should fix that handling here if needed.

What I don't follow is why we need to stop optimizing reachable cases. The first part of the test diff (where my comment is) looks like a regression. Can we not avoid that?

[kripken]

This was valid before, so this looks like a regression to me? I may be missing the reason you say this change is needed, sorry.

[tlively]

Closing this in favor of #7117