ExceptionRegion modeling issues and proposed improvements

[Adam Sotona]

Here is lifted version of the same code, where 11 more synthetic blocks are necessary to exit the exception regions correctly:

func @"nestedCatch" (%0 : int)void -> {
    branch ^block_1;

  ^block_1:
    %1 : java.lang.reflect.code.op.CoreOp$ExceptionRegion = exception.region.enter ^block_2 ^block_27;

  ^block_2:
    %2 : java.lang.reflect.code.op.CoreOp$ExceptionRegion = exception.region.enter ^block_3 ^block_20;

  ^block_3:
    %3 : int = constant @"0";
    %4 : boolean = eq %0 %3;
    cbranch %4 ^block_4 ^block_7;

  ^block_4:
    exception.region.exit %2 ^block_5;

  ^block_5:
    exception.region.exit %1 ^block_6;

  ^block_6:
    return;

  ^block_7:
    exception.region.exit %2 ^block_8;

  ^block_8:
    exception.region.exit %1 ^block_9;

  ^block_9:
    %5 : java.lang.reflect.code.op.CoreOp$ExceptionRegion = exception.region.enter ^block_10 ^block_27;

  ^block_10:
    %6 : java.lang.reflect.code.op.CoreOp$ExceptionRegion = exception.region.enter ^block_11 ^block_19;

  ^block_11:
    %7 : int = constant @"1";
    %8 : boolean = eq %0 %7;
    cbranch %8 ^block_12 ^block_15;

  ^block_12:
    exception.region.exit %6 ^block_13;

  ^block_13:
    exception.region.exit %5 ^block_14;

  ^block_14:
    branch ^block_1;

  ^block_15:
    exception.region.exit %6 ^block_16;

  ^block_16:
    exception.region.exit %5 ^block_17;

  ^block_17:
    %9 : java.lang.reflect.code.op.CoreOp$ExceptionRegion = exception.region.enter ^block_18 ^block_27;

  ^block_18:
    exception.region.exit %9 ^block_24;

  ^block_19(%10 : java.lang.NullPointerException):
    exception.region.exit %5 ^block_21(%10);

  ^block_20(%11 : java.lang.NullPointerException):
    exception.region.exit %1 ^block_21(%11);

  ^block_21(%12 : java.lang.NullPointerException):
    %13 : java.lang.reflect.code.op.CoreOp$ExceptionRegion = exception.region.enter ^block_22 ^block_27;

  ^block_22:
    invoke %12 @"java.lang.NullPointerException::printStackTrace()void";
    exception.region.exit %13 ^block_23;

  ^block_23:
    branch ^block_24;

  ^block_24:
    %14 : java.lang.reflect.code.op.CoreOp$ExceptionRegion = exception.region.enter ^block_25 ^block_27;

  ^block_25:
    invoke @"javaapplication6.NewClass10::doSomething()void";
    exception.region.exit %14 ^block_26;

  ^block_26:
    branch ^block_1;

  ^block_27(%15 : java.lang.RuntimeException):
    invoke %15 @"java.lang.RuntimeException::printStackTrace()void";
    branch ^block_1;
};


> While this looks more correct, I don't really get the exception region exits in block_7 and block_8 (only to be re-entered again).

That is exactly the complexity I'm talking about. A fragmented exception region mandatory dominated by its entry cause it to split similar to what SSA does to multiple-assigned variables.

> The second exception region has the handler at block_19 and block_20.

Yes, that is also enforced by the current model spec. A NPE thrown and handled in in block_19 needs to first leave exception.region.exit %5, while in block_20 it leaves a different region entry and in block_21 it is technically out of the regions. I don't think the lifting algorithm is optimal, however proposed change clearing the exception stack when thrown will definitely remove this synthetic "leaving the exception region" blocks.

> My feeling is that these little discrepancies add up quite a lot... if we could see that e.g. we leave the regions in 7 and 8 only to reenter them again in 9 and 10, then surely we can omit these redundant blocks (given they don't seem to be reachable from anywhere else). Also, the duplication between block 19 and 20 is annoying as well.

You may try to implement it as a post-lift compaction transform or try to embed some optimizations it into the lift, however the complexity is like an attempt to lift locals into variables directly in SSA form. That is why I'm proposing to remove the "single dominant exception entry" condition from the spec and replace it with the condition that entries to an exception region form a dominant set, dominating to the exits from the region.


With proposed changes the above example model may look like this:

func @"nestedCatch" (%0 : int)void -> {
    %1 : java.lang.reflect.code.op.CoreOp$ExceptionRegion<java.lang.RuntimeException> = exception.region ^block_13;
    %2 : java.lang.reflect.code.op.CoreOp$ExceptionRegion<java.lang.NullPointerException> = exception.region ^block_9;
    branch ^block_1;

  ^block_1:
    exception.region.enter %1 %2 ^block_2; // entering nested try blocks

  ^block_2:
    %3 : int = constant @"0";
    %4 : boolean = eq %0 %3;
    cbranch %4 ^block_3 ^block_5;

  ^block_3:
    exception.region.exit %2 %1 ^block_4; // leaving nested try blocks

  ^block_4:
    return;

  ^block_5:
    %5 : int = constant @"1";
    %6 : boolean = eq %0 %5;
    cbranch %6 ^block_6 ^block_8;

  ^block_6:
    exception.region.exit %2 %1 ^block_7; ; // leaving nested try blocks

  ^block_7:
    branch ^block_1;

  ^block_8:
    exception.region.exit %2 ^block_11;  // leaving only the inner NPE try block

  ^block_9(%7 : java.lang.NullPointerException):
    exception.region.enter %1 ^block_10; // re-entering the outer try block

  ^block_10:
    invoke %7 @"java.lang.NullPointerException::printStackTrace()void";
    branch ^block_11;

  ^block_11:
    invoke @"javaapplication6.NewClass10::doSomething()void";
    exception.region.exit %1 ^block_12; // leaving the outer try block

  ^block_12:
    branch ^block_1;

  ^block_13(%8 : java.lang.RuntimeException):
    invoke %8 @"java.lang.RuntimeException::printStackTrace()void";
    branch ^block_1;
};


> This does look better - but in part also because some of the problems highlighted above have been addressed? (e.g. more sharing of code paths, avoid duplicted exception region...)

Yes, the exception regions can be deduplicated, because they are no more single-entry dominant the exits.

Thanks,

Adam

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/babylon-dev/attachments/20241009/a17fe488/attachment-0001.htm>