refactor[cartesian]: avoid extra boolean if we already have a boolean

[romanc]

Description

This is little cleanup in the dace backends regarding conditionals, i.e. what is know in in the oir as MaskStmt.

Mask statements "outsource" the evaluation of conditionals to a separate tasklets to avoid DaCe issues down the line. However, most of the time, the mask is already a boolean scalar (gtir -> oir makes such that field accesses are split out into a separate assignment (read here if you are interested)).

With this PR, we skip the step to make yet another temp boolean in oir_to_treeir if we detect a plain boolean scalar access (if my_boolean) or a negated boolean scalar (if not my_boolean). In the upstream case of DelnFlux, this changes the beginning of the generated SDFG as follows (left before, right after)

left: with this PR | right: before this PR

Notice how before we have more tasklets copying one boolean into another where on the left side (after) we only have one tasklet left (which originates from the above gtir -> oir translation step).

Requirements

• All fixes and/or new features come with corresponding tests.
  Assumed to be covered by the existing test suite.
• Important design decisions have been documented in the appropriate ADR inside the docs/development/ADRs/ folder. N/A

[romanc]

Actually, I've been thinking about this in the evening: Since we already split field accesses in gtir -> oir, we can always omit the extra tasklet in oir -> treeir because only field access has the potential of indirect field access. We are thus safe when it comes to

https://github.com/spcl/dace/blob/7fa7591c6f122dcfa396a02ff3a2b9b504126887/dace/sdfg/sdfg.py#L205

I don't know/remember what is meant by "sympy issues down the line" (comment in code), but I think, we should just try and see if anything still breaks. This might have been magically solved by the DaCe v2 update ...

[romanc]

I don't know/remember what is meant by "sympy issues down the line" (comment in code), but I think, we should just try and see if anything still breaks. This might have been magically solved by the DaCe v2 update ...

So I tried and eventually hit "sympy issues down the line": We are apparently (still) very limited in what we can use as conditionals. Consider the following stencil fragment

    def definition(field_a, field_b, field_c, *, factor):
        with computation(PARALLEL), interval(...):
            if factor > 0:
                ...

where field_{a,b,c} are fields and factor is a scalar. If we are to blindly put factor > 0 into the condition, we end up with an SDFG (segment) that looks like this

parsing converts 0 -> int64_t(0) to ensure literal int precision and then adds a double cast to ensure we are comparing apples to apples with the factor that is in double precision (configured from literal float precision). So far so good (no complaints from sdfg.validate()) until we try to simplify this SDFG, which will kick off an analysis pass called AccessSet (basic building block of some simplify passes). This pass will then go and as part of its analysis try to "sympify" the condition factor > double(int64_t(0)). That "sympification" fails because the condition can't be evaluated because sympy has no clue about double() and int64_t() casts.

There are ways around this now because dace added these rust-like "literals with precision information", so we could generate those. Not sure how well they would "sympify" and I'm not keen on finding out.

I suggest we stay with

• if it's a boolean scalar access: just use it
• if it's and expression and then expression is not boolean_scalar; then use that
• otherwise keep the "evaluation tasklet" until - at some point - we really clean up this mess.

[FlorianDeconinck]

I don't know/remember what is meant by "sympy issues down the line" (comment in code), but I think, we should just try and see if anything still breaks. This might have been magically solved by the DaCe v2 update ...

So I tried and eventually hit "sympy issues down the line": We are apparently (still) very limited in what we can use as conditionals. Consider the following stencil fragment

    def definition(field_a, field_b, field_c, *, factor):
        with computation(PARALLEL), interval(...):
            if factor > 0:
                ...

where field_{a,b,c} are fields and factor is a scalar. If we are to blindly put factor > 0 into the condition, we end up with an SDFG (segment) that looks like this

parsing converts `0` -> `int64_t(0)` to ensure literal int precision and then adds a `double` cast to ensure we are comparing apples to apples with the `factor` that is in double precision (configured from literal float precision). So far so good (no complaints from `sdfg.validate()`) until we try to simplify this SDFG, which will kick off an analysis pass called `AccessSet` (basic building block of some simplify passes). This pass will then go and as part of its analysis try to "sympify" the condition `factor > double(int64_t(0))`. That "sympification" fails because the condition can't be evaluated because `sympy` has no clue about `double()` and `int64_t()` casts.

There are ways around this now because dace added these rust-like "literals with precision information", so we could generate those. Not sure how well they would "sympify" and I'm not keen on finding out.

I suggest we stay with

• if it's a boolean scalar access: just use it
• if it's and expression and then expression is not boolean_scalar; then use that
• otherwise keep the "evaluation tasklet" until - at some point - we really clean up this mess.

Bit frustrating but fair enough. I think I want to go to the rust-like literal at some point anyway, if nothing but to follow the mantra of "use the framework as intended/as the majority does".

Can you log that in NDSL for tracking? We will revisit this then.

[romanc]

Bit frustrating but fair enough. I think I want to go to the rust-like literal at some point anyway, if nothing but to follow the mantra of "use the framework as intended/as the majority does".

Sounds good. I have added the second case for the unary operator of a scalar boolean. With this, we at least get the redundant tasklets and booleans out of the way.

Can you log that in NDSL for tracking? We will revisit this then.

Logged in NOAA-GFDL/NDSL#552.

[romanc]

This is ready for review in its final form. CSCS-CI will be out all day due to maintenance.

[romanc]

Cscs-ci run default

[romanc]

cscs-ci run default

[romanc]

cscs-ci run default