The original diff file was from D13974 for the purpose of comparison. This is the updated diff file.

ping...

Update patch w.r.t. Philip's comments.

Update patch w.r.t Philip's comments.

@rnk -- as for ExceptionHandling.rst, I'd prefer to update gc.statepoint document first on how they use landingpads of token type, and then add a reference in ExceptionHandling.rst. Does that sounds reasonable to you?

Update patch with initial approach as described by the updated summary.

In D15405#310327, @JosephTremoulet wrote:

In D15405#310301, @rnk wrote:

In D15405#310289, @JosephTremoulet wrote:

It just seemed backwards to snoop the landingpad's type when we expect landingpad type to be dictated by the personality, nothing deeper than that.

I think it's fine. landingpad personalities don't impose a lot of constraints on the compiler. It seems nicer to treat them as more of an open set of well-behaved things.

Alright. Chen, sorry for the back-and-forth.

I can imagine wanting to use some kind of relocating GC scheme in a function that uses C++ EH, and needing to name out the right __gxx_personality* symbol for the platform.

Update patch to make the new enum similar to Unknown in all cases, except that it has landingpads of token types.

If you don't like adding your runtime's personality to the enum, perhaps we should go back to Chen's original suggestion of just checking the type of the landingpad in SelectionDAG. The reason we started going in this direction instead was "It seems a little backwards to check the landingpad's type as the first check, since the personality dictates the landingpad's type". But maybe it makes sense to allow that much variation within Unknown, since it is after all a bucket for "all the other personalities" -- if we wanted to be pedantic, SelectionDAG could allow either token or struct for Unknown, but require struct for all the known personalities that use landingpads.

Add assertions to make sure overflow never happens.

Ah, ok. That makes sense, though it means that "similar to Unknown, except that LandingPads have token type" isn't exactly correct as a description for Token_LP. Since you're wanting both token-valued landingpads and to opt into isNoOpWithoutInvoke, I wonder if it wouldn't be better to go ahead and be specific about your runtime like the other non-Unknown enum values ... call it AzulJVM or whatever in the enum and use the name of an appropriate function in your runtime as the string. If you want the code in SelectionDAG to be more general, you could add a property like usesTokenLandingPads(EHPersonality Pers) to EHPersonalities.h which returns true for AzulJVM, and have SelectionDAG query through that.

In D15405#307177, @JosephTremoulet wrote:

We want the new personality to generally be treated like Unknown, correct? You should check for places in the code that look for EHPersonality::Unknown and make sure they agree for Token_LP. From a quick look, I think you'd want to update isNoOpWithoutInvoke in EHPersonalities.h and isCatchAll in InstructionCombining.cpp.

I applied the patch locally and it worked with 0xFFFFFFFF. However, in practice, neither 0xFFFFFFFF nor 0 will flow into this function because llvm handles them with special cases. For 0, it returns a 0 directly, and for 0xFFFFFFFF, it returns 0 - (the other operand). I've hacked to bypass the special cases in order to test the overflow case could be handled correctly by this code.

I applied the patch locally and it worked with 0xFFFFFFFF. However, in practice, neither 0xFFFFFFFF nor 0 will flow into this function because llvm handles them with special cases. For 0, it returns a 0 directly, and for 0xFFFFFFFF, it returns 0 - (the other operand). I've hacked to bypass the special cases in order to test the overflow case could be handled correctly by this code.

ping...

In D15315#304816, @RKSimon wrote:

Just to be clear - APInt operators don't prevent or assert on overflow, it just provides *_ov methods that make it easier to recognise when overflow occurs. Please can you confirm that this approach will work for D14603?

Update patch w.r.t Philip's suggestions.

First, fix the issues mentioned by David.

Update w.r.t Philip's and David's comments.

Update patch w.r.t Sanjoy's comments.

Update patch w.r.t Sanjoy's comments.

Correct test command line options.

Update the patch so it does not crash bug 25299.

Update patch w.r.t Sanjoy's request.

Update patch.

Update patch w.r.t Philip's comments.

Update patch w.r.t Philip's comments

In D13718#267109, @reames wrote:

The general transform looks reasonable, but I'd suggest possibly tackling this a slightly different way. Rather than explicitly doing the detection of a trivial block and rewrite in a single step, replace the branch to the unified resume with a resume and then let the existing code handle it. Doing it this way could make the change less invasive and easier to reason about. Note that this is a suggestion to think about, not a requirement. If you feel the current approach is easier/better, we can run with this after cleanup.