[code-reflection] RFR: [hat][codegen] Parenthesis in Expressions for binaryOps fixed [v2]

[Gary Frost]

On Mon, 22 Sep 2025 14:41:15 GMT, Juan Fumero <jfumero at openjdk.org> wrote:

>> HAT Code gen was not generated parenthesis correctly. This PR forces to use an open and closed parenthesis for binaryOps. 
>> 
>> Code reflection generates code models in SSA form. Therefore, priority is given by computing values first the values before carrying out the result-op to other ops. 
>> 
>> For instance: 
>> 
>> 
>> final int TN = 2;
>> final int TF = 128;
>> final int MAX = 1024;
>> int c = MAX / (TN * TF);
>> 
>> 
>> Generates the following code model:
>> 
>> 
>>     %10 : java.type:"int" = var.load %9 @loc="50:17";
>>     %11 : java.type:"int" = var.load %5 @loc="50:24";
>>     %12 : java.type:"int" = var.load %7 @loc="50:29";
>>     %13 : java.type:"int" = mul %11 %12 @loc="50:24";    >> mult
>>     %14 : java.type:"int" = div %10 %13 @loc="50:17";      >> div
>>     %15 : Var<java.type:"int"> = var %14 @loc="50:9" @"c";
>> 
>> 
>> while 
>> 
>> 
>> final int TN = 2;
>> final int TF = 128;
>> final int MAX = 1024;
>> int c = MAX / TN * TF;
>> 
>> 
>> Generates the following code model.
>> 
>> 
>>     %10 : java.type:"int" = var.load %9 @loc="50:17";
>>     %11 : java.type:"int" = var.load %5 @loc="50:23";
>>     %12 : java.type:"int" = div %10 %11 @loc="50:17";
>>     %13 : java.type:"int" = var.load %7 @loc="50:28";
>>     %14 : java.type:"int" = mul %12 %13 @loc="50:17";
>>     %15 : Var<java.type:"int"> = var %14 @loc="50:9" @"c";
>> 
>> 
>> The issue was that in the HAT codegen, parentheses were computed based on pure precedence of the operator, not based on the dependencies. 
>> 
>> How to test:
>> 
>> 
>> HAT=SHOW_CODE java @hat/test ffi-opencl oracle.code.hat.TestParenthesis
>
> Juan Fumero has updated the pull request incrementally with one additional commit since the last revision:
> 
>   [hat] More tests to check nested parenthesis

parenthesisIfNeeded() gets passed the parent and child operators (Ops) so  full access to the model is available.  

As I mentioned in slack, it does not at present take 'associativity' into account.  Most arith/logic are left to right associative.  So my initial thought is that we first check precedence (as we do from the Spec) and then decide whether we need parenthesis based on the associativity of the parent vs child op.   Assuming that this order is preserved by the model (which I suspect it is). 

As I mentioned..  I suspect this is just a bug in parenthesisIfNeeded() not grouping left to right.  It smells almost like the precedence test (for most ops) is using < instead of <= ;) 

See 
    public static boolean needsParenthesis(Op parent, Op child) {
        return OpTk.precedenceOf(parent) < OpTk.precedenceOf(child);
    }

-------------

PR Comment: https://git.openjdk.org/babylon/pull/579#issuecomment-3319964716