Make tailrec label optional

[xclerc]

This pull request changes the type of the fun_tailrec_entry_point_label
field from Label.t to Label.t option. The rationale is that the block
acting as the "tailrec" entry point can be deleted by the fallthrough/dead
code optimization, leaving the field to point to a block not present in
the blocks field. While this did not trigger any issue in the current code,
it appears safer to keep the fun_tailrec_entry_point_label and blocks
fields consistent with each other.

[gretay-js]

This leaves Linear.fundecl.fun_tailrec_entry_point_label dangling if the "tailrec" block is eliminated.
We can resolve it by making the field in Linear optional.

[xclerc]

I naïvely expected the compiler to spot such an issue...
The culprit is, obviously, this expression; are we sure the
other fields will never be in need of an update?

26ae9da is propagating the change to Linear.fundecl.

[xclerc]

Actually, I am not sure this change is correct.

Could not we be in a situation were we remove an
empty "tailrec" block but still have recursive tail calls?

We can probably not hit this case currently, as
Linearize.linear will not produce empty blocks.

[gretay-js]

In the case of Tailcall(Self), t.fun_tailrec_entry_point_label must not be None, so Option.map is fine, but may be worth checking.

[gretay-js]

The culprit is, obviously, this expression; are we sure the
other fields will never be in need of an update?

Yes, I expect that we will update most of the other fields. Ocamlcfg.Cfg_with_layout.to_linear should return Linear.fundecl instead of Linear.instruction.

[gretay-js]

This change makes it explicit whether the tailrec entry is used or not. Can you please add a check that sets this field correctly (see comments)?

Could not we be in a situation were we remove an empty "tailrec" block but still have recursive tail calls?

yes, it's possible if tailrec block is a fallthrough block and empty. I think this is common and i think this PR correctly updates fun_tailrec_entry_point_label of the CFG in this case. Could we have a small test for it?

It is not likely that the tailrec block is dead. but we should either have an assert false in that case or handle it correctly.

[gretay-js]

I think this check is not correct here: if there is another Tailcall Self that is not dead, we cannot set fun_tailrec_entry_point_label to None. Instead, we should check at the end of CFG construction and transformations whether any block has Tailcall Self and if not, then we can set fun_tailrec_entry_point_label to None.

[gretay-js]

sorry, this is actually fine and I was confused about who is dead! I still think it's better to do this check at the end.

[xclerc]

I still think it's better to do this check at the end.

I am slightly confused: it looks like the operation is the very
last of the function, and hence the whole transformation.

[gretay-js]

This check will happen at the end of each recursive call.
What I meant is to have a separate pass that checks if the input Cfg has any Tailcall Self, and if it does not, sets tailrec label to None. This pass can be called at the end of Linear_to_cfg.run, and at the end of Eliminate_dead_blocks.run and at the end of Eliminate_fallthrough_blocks.run.
What you have here is more efficient, because it fixes up tailrec label only as needed, but I think we have this problem in the first place because the representation of tailrec label is problematic.

As suggested offline, this problem would completely go away if we change the representation of Tailcall Self to include the label, and also remove fun_tailrec_entry_point_label in Cfg. It is still worth changing Linear.fundecl.fun_tailrec_entry_point_label to an option. The pass Cfg_to_linear can compute the field and check its consistency (i.e., all Tailrec Self have the same destination label).

[xclerc]

I understand your point about the recursive calls.
However, it sounds simpler to have these calls accumulate
the set of dead blocks rather than have a whole new phase/
traversal at the end of run. run would get the set of
eliminated blocks from eliminate_dead_blocks and the
code currently at lines 68-73 would be at the end of run.

[gretay-js]

Same as above, I think this check should be done at the end of the entire transformation.

Unlike the above case, this seem correct for a subtle reason: if tailrec block was a fallthrough and got eliminated, then either (a) there are Tailcall Self, and then the label here will be the new fun_tailrec_entry_point_label which is the successor of the original one, so won't be in found or (b) there are no Tailcall Self, and the label is not used and can be safely set to None.

[xclerc]

Independently of the discussion about the proposed code,
do we think that removing the notion of tailrec calls(thus
replacing such calls with mere jumps) should happen?

I am always a bit wary of removing structure from a
representation, but the only real reason I am not sure
whether this change is a good idea it related to the
emission of DWARF info. Would DWARF allow us to mark
a jump as actually being a tailrec call?

@gretay-js @mshinwell

[gretay-js]

My original proposal of replacing "Tailcall Self" with jump was wrong, as you point out, because it wouldn't work with DWARF. The current proposal is to add the destination label to the terminator itself, instead of storing (and sharing) the label at function level.

[xclerc]

Sorry, I missed this change. Then, there is no loss of information.

[xclerc]

(Made obsolete by #51.)