@lthls : would you like to review this one as well?

Thanks @lthls. I've added some words about the notify_catch callback technique.

I'll ask a few people around me who have experienced with register types if there's anything to watch for.

I think all the cmm and post-cmm backend is ready to deal with non-val types (also for functions arguments, which is what I used in an experimental PR about unboxed calling conventions), but indeed, this hasn't received wide coverage. Did you more feedback from other people?

I'd be curious to know how often the No_result case occurs in transl_catch, and on which kind of programs.

I haven't been able to produce this situation (corresponding to a dead catch handler), and possibly this is excluded by other passes, but it would seem weird to reject the case at this point.

I think all the cmm and post-cmm backend is ready to deal with non-val types (also for functions arguments, which is what I used in an experimental PR about unboxed calling conventions), but indeed, this hasn't received wide coverage. Did you more feedback from other people?

Yes, they confirmed that they didn't find any bugs linked to this change in their experiments.

I haven't been able to produce this situation (corresponding to a dead catch handler), and possibly this is excluded by other passes, but it would seem weird to reject the case at this point.

I wasn't thinking about rejecting the case, I would have simply removed the handler since it's unreachable. If you haven't encountered such a situation in practice, it's probably not worth worrying about.

Thanks for addressing my concerns.

In the light of the recently merged #8584, do you want to take the opportunity to show some of the improvements that one can expect ? Your original pull request (#2162) showed some of the cases where it would trigger, maybe you could build a micro-benchmark based on these.

Sorry, I should indeed have put a link to #2165 (comment) which still applies (just checked).

(That's a 40% speedup on the micro-benchmark.)

Based on @lthls 's approval and the availability of the micro-benchmark above, I'll merge this soon if nobody complains.

@alainfrisch Have you obtained any benchmark numbers from larger programs with this patch? It seems a significant enough change to be worth doing so.

As an aside, I think the existing register typing for unboxed integers is dubious, as they are assigned type Int. However this could in theory be joined with Val to yield Val, which would be wrong for e.g. a boxed int64. I suspect such a join never happens for a register holding a value that arose from unboxing, but this should probably be fixed nonetheless. My previous PR on improved register typing, which has been taken over by @Gbury (in conjunction with work on register type inference, as discussed at the last dev meeting), should be sufficient.

@alainfrisch Have you obtained any benchmark numbers from larger programs with this patch? It seems a significant enough change to be worth doing so.

No visible change on some larger payloads I tried, but we wrote our critical numerical code knowing how unboxing works currently, and carefully rewrote parts to workaround current limitations. This ends up with some code like:

  let a = ref 0. and b = 0. in
  if cond then (a := ...; b := ...) else (a:= ...; b := ...)

which could be avoided by a simple unboxed float tuple binding with this PR.

I suspect such a join never happens for a register holding a value that arose from unboxing, but this should probably be fixed nonetheless.

If there is a problem, I think it already exists with normal unboxing of let-bound identifiers. Here we unbox catch arguments if they are boxed numbers for sure, considering all call sites; and for the "continuation" (i.e. the handler code), it is as if we inserted a local let-bound identifier to hold the unboxed value (let argi_unboxed = unbox argi in ...).

I think the typing problem isn't specific to this PR, agreed.

@mshinwell : are you ok with merging?

I think as long as you're satisfied with the potential performance changes, it's fine.

Closed/reopened to relaunch Appveyor CI, which took significantly longer than usual (and one job was killed because of it); no problem on Travis, so I suspect a transient issue on Appveyor, but let's be sure before merging.

Ok, AppVeyor finished each job in < 1 hour, but job time seems to be significantly higher than on master; I think it's worth investigating before merging.

False alarm, the latest AppVeyor build is a fast as usual (also confirmed by checking AppVeyor logs that slow builds were also slow on the bytecode parts).