NewRadX: Fix reads and writes in emited schedule

[lucass-carneiro]

Currently, given a variable u and its RHS u_rhs, in which NewRadX is applied, we emit schedule statements such as

SCHEDULE NewRadXBoundaryFn_0 IN Foo AFTER Bar
{    
    LANG: C
    Reads: u(Interior)
    Writes: u_rhs(Interior)
} "Desc"

Due to how NewRadX_Apply works, we need to emit

SCHEDULE NewRadXBoundaryFn_0 IN Foo AFTER Bar
{    
    LANG: C
    Reads: u(Interior)
    Reads: u_rhs(Interior)
    Writes: u_rhs(Interior)
} "Desc"

[rhaas80]

Anyone? @chcheng3 ? Alex?

[chcheng3]

I can see the additiononal Reads: u_rhs(Interior) making the schedule clearer by being more explicit. However, I wonder if this change actually makes a difference for CarpetX. Naively I would think that by declaring that a grid function to be written, then a read must also happen, so it wouldn't be strictly necessary to add a Reads: if there is already a Writes:. Am I understanding it correctly?

[rhaas80]

Fortran makes that distinction, you can have output-only subroutine arguments. It's just in C / C++ that one cannot model this. Note though that CarpetX if poisoning is turned on and a grid function only has write clauses, will poison the grid function before calling the scheduled function (you claimed you were not going to read from it, so CarpetX will make very sure there is nothing to read...).

So: makes a difference :-)

[chcheng3]

That makes sense, thanks for the correction! Would this distinction apply to all CarpetX thorns then?

[rhaas80]

yes, if you actually rely on data in the grid function being valid on entry then you must add the "READS". If you make data valid, you should set "WRITES".

[chcheng3]

Okay, I was confused because I was not thinking of it from this perspective before, but this makes it much more clear now. I think it should be made more clear in the CarpetX documentation how the reads, writes, and poisoning mechanism work together.

[rhaas80]

ok, maybe we should have called them "REQUIRES" and "PROVIDES". Too late now.

[lucass-carneiro]

Ok, I was looking at the tickets for NewRadX and I realized that I wanted to have opened this ticket in the EinsteinEngine code generator repository, not here 🤦

I don't think there's anything NewRadX should do really, besides let users know that they need to do that when calling the API. As @rhaas80 said, I have encountered this issue when testing with poisoning enabled, so the schedule statements are required.

If there's a documentation for NewRadX, this info should go there, and as @chcheng3 said, we should talk about this in the CarpetX documentation. Can anyone put in a ticket in the ETK issue tracker please so that I (or someone else) remembers to do that and link this issue?