compiler: Add guards to prevent OOB when streaming buffers with ConditionalDimension #2150

ccuetom · 2023-06-22T15:46:13Z

Using the time derivative of a streamed buffer with a ConditionalDimension results in OOB within init_to_host. This was not captured in existing tests.

A new test has been added test_gpu_common.py::test_streaming_multi_input_conddim_foward that fails with this OOB; and a new guard is now introduced in Buffering to prevent it.

mloubout · 2023-06-23T15:48:59Z

tests/test_gpu_common.py

+        for i in range(u.save):
+            u.data[i, :] = i
+
+        expr = u.dt2 + 3.*u.dt(x0=time_sub - time_sub.spacing)


FYI this is just u.dtl

good to know - I adapted the code from the next test there

mloubout

Looks good to me, is there some standard cond dim cases that were broken as well non related to your issue?

mloubout · 2023-06-23T15:50:00Z

tests/test_buffering.py

@@ -589,7 +589,7 @@ def test_conddim_w_shifting():
    op1 = Operator(eqns, opt='buffering')

    # Check generated code
-    assert len(retrieve_iteration_tree(op1)) == 3
+    assert len(retrieve_iteration_tree(op1)) == 4


So what changed in the code since it seems to change existing tests not related to init_to_host

These tests also generate initialisation code that is affected by the addition of the guard. The loop:

if (time_m <= time_M) { /* Begin section0 */ START_TIMER(section0) for (int db0 = 0; db0 <= 0; db0 += 1) { for (int x = x_m; x <= x_M; x += 1) { for (int y = y_m; y <= y_M; y += 1) { ub0[(db0 + (time_M / factor))%(1)][x][y] = u[db0 + (time_M / factor)][x][y]; } } } STOP_TIMER(section0,timers) /* End section0 */ }

becomes:

if (time_m <= time_M) { /* Begin section0 */ START_TIMER(section0) for (int db0 = 0; db0 <= 0; db0 += 1) { if (0 <= db0 + (time_M / factor)) { for (int x = x_m; x <= x_M; x += 1) { for (int y = y_m; y <= y_M; y += 1) { ub0[(db0 + (time_M / factor))%(1)][x][y] = u[db0 + (time_M / factor)][x][y]; } } } } STOP_TIMER(section0,timers) /* End section0 */ }

is that what causes the bump from 3 to 4 iteration nests?

this is the iteration nest before adding the guard:

IterationTree(<WithProperties[affine,parallel]::Iteration db0; (0, 0, 1)>, <WithProperties[affine,parallel]::Iteration x; (x_m, x_M, 1)>, <WithProperties[affine,parallel]::Iteration y; (y_m, y_M, 1)>) IterationTree(<WithProperties[affine,sequential]::Iteration time[t0,t1]; (time_m, time_M, 1)>,) IterationTree(<WithProperties[affine,sequential]::Iteration time[t0,t1]; (time_m, time_M, 1)>, <WithProperties[affine,parallel,parallel=]::Iteration x; (x_m, x_M, 1)>, <WithProperties[affine,parallel,parallel=]::Iteration y; (y_m, y_M, 1)>)

and after adding the guard:

IterationTree(<WithProperties[affine,parallel]::Iteration db0; (0, 0, 1)>,) IterationTree(<WithProperties[affine,parallel]::Iteration db0; (0, 0, 1)>, <WithProperties[affine,parallel]::Iteration x; (x_m, x_M, 1)>, <WithProperties[affine,parallel]::Iteration y; (y_m, y_M, 1)>) IterationTree(<WithProperties[affine,sequential]::Iteration time[t0,t1]; (time_m, time_M, 1)>,) IterationTree(<WithProperties[affine,sequential]::Iteration time[t0,t1]; (time_m, time_M, 1)>, <WithProperties[affine,parallel,parallel=]::Iteration x; (x_m, x_M, 1)>, <WithProperties[affine,parallel,parallel=]::Iteration y; (y_m, y_M, 1)>)

Iteration tree IterationTree(<WithProperties[affine,parallel]::Iteration db0; (0, 0, 1)>,) appears as a result. Not sure if this is expected or if you want me to probe into it.

no, it's OK!

ccuetom · 2023-06-23T15:56:20Z

Looks good to me, is there some standard cond dim cases that were broken as well non related to your issue?

@mloubout Issue popped up with personal code that does something similar to this, so I haven't seen anything else that broke

codecov · 2023-06-23T16:10:43Z

Codecov Report

Merging #2150 (746afeb) into master (3353683) will decrease coverage by 0.04%.
The diff coverage is 54.76%.

@@            Coverage Diff             @@
##           master    #2150      +/-   ##
==========================================
- Coverage   87.10%   87.07%   -0.04%     
==========================================
  Files         223      223              
  Lines       39893    39916      +23     
  Branches     5172     5173       +1     
==========================================
+ Hits        34750    34758       +8     
- Misses       4567     4580      +13     
- Partials      576      578       +2

Impacted Files	Coverage Δ
tests/test_gpu_common.py	`1.44% <0.00%> (-0.04%)`	⬇️
devito/passes/clusters/buffering.py	`92.15% <100.00%> (+0.08%)`	⬆️
tests/test_buffering.py	`98.70% <100.00%> (ø)`

... and 3 files with indirect coverage changes

FabioLuporini · 2023-06-28T07:19:18Z

devito/passes/clusters/buffering.py

@@ -177,6 +179,9 @@ def callback(self, clusters, prefix, cache=None):
                lhs = b.indexed[dims]._subs(dim, b.firstidx.b)
                rhs = b.function[dims]._subs(dim, b.firstidx.f)

+                if dim.is_Conditional:


what about we place the guard regardless of whether it's a ConditionalDimension or a plain Dimension?

second question.

Since db0 starts, by construction, at 0 and can only take positive values, how about this as a guard instead:

guards[pd] = GuardBound(0, b.firstidx.f - b.xd) # aka: time_M / factor >= 0

then later on, you would have to use andg to chain this guard with the other time_M >= time_m guard

this would generalise to the no-ConditionalDimension case as well

this way, we would generate neater code in my opinion

I might be missing something, but I think the guard does need to be on db0 - we need to check that db0 + (time_m / factor) - 1 >= 0 or db0 + (time_M / factor) - 1 >= 0 depending on whether it's forward or backward.

Using guards[pd] = GuardBound(0, b.firstidx.f - b.xd) # aka: time_M / factor >= 0 tests fail as expected.

I think you're right I just got confused

However, I still think this generalisation is feasible:

what about we place the guard regardless of whether it's a ConditionalDimension or a plain Dimension?

Yes, that should not be a problem. It will make more tests trip on the number of iteration trees, but happy to make the changes

FabioLuporini · 2023-06-28T12:57:30Z

devito/passes/clusters/buffering.py

@@ -226,8 +231,13 @@ def callback(self, clusters, prefix, cache=None):
                ispace = b.readfrom
                properties = c.properties.sequentialize(d)

+                guards = c.guards


nitpicking: guards are typically computed before properties

FabioLuporini

Thanks for all these changes. Appreciate that. Approved!

mloubout · 2023-07-05T12:10:07Z

Rebase and GTG

…tionalDimension

FabioLuporini · 2023-07-07T12:27:55Z

Many thanks! Merging...

mloubout reviewed Jun 23, 2023

View reviewed changes

mloubout added the bug-py-minor label Jun 23, 2023

FabioLuporini reviewed Jun 28, 2023

View reviewed changes

FabioLuporini approved these changes Jul 4, 2023

View reviewed changes

mloubout approved these changes Jul 5, 2023

View reviewed changes

ccuetom added 5 commits July 5, 2023 15:41

compiler: Add guards to prevent OOB when streaming buffers with Condi…

9c41f7c

…tionalDimension

compiler: Update buffering tests to match new trees

7d41ca5

compiler: Correct guards update in buffering

67c69f8

compiler: Fix line lengths

8d8fbcb

compiler: Apply buffering guards to non-ConditionalDimension

746afeb

ccuetom force-pushed the add-streaming-guards branch from 5903ad1 to 746afeb Compare July 5, 2023 14:49

FabioLuporini merged commit 474b10c into devitocodes:master Jul 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compiler: Add guards to prevent OOB when streaming buffers with ConditionalDimension #2150

compiler: Add guards to prevent OOB when streaming buffers with ConditionalDimension #2150

ccuetom commented Jun 22, 2023

mloubout Jun 23, 2023

ccuetom Jun 23, 2023

mloubout left a comment

mloubout Jun 23, 2023

ccuetom Jun 23, 2023 •

edited

Loading

FabioLuporini Jun 28, 2023

ccuetom Jun 28, 2023

FabioLuporini Jun 28, 2023

ccuetom commented Jun 23, 2023 •

edited

Loading

codecov bot commented Jun 23, 2023 •

edited

Loading

FabioLuporini Jun 28, 2023

FabioLuporini Jun 28, 2023

ccuetom Jun 28, 2023

FabioLuporini Jun 28, 2023

ccuetom Jun 28, 2023

FabioLuporini Jun 28, 2023

FabioLuporini left a comment

mloubout commented Jul 5, 2023

FabioLuporini commented Jul 7, 2023

compiler: Add guards to prevent OOB when streaming buffers with ConditionalDimension #2150

compiler: Add guards to prevent OOB when streaming buffers with ConditionalDimension #2150

Conversation

ccuetom commented Jun 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mloubout left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccuetom Jun 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccuetom commented Jun 23, 2023 • edited Loading

codecov bot commented Jun 23, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FabioLuporini left a comment

Choose a reason for hiding this comment

mloubout commented Jul 5, 2023

FabioLuporini commented Jul 7, 2023

ccuetom Jun 23, 2023 •

edited

Loading

ccuetom commented Jun 23, 2023 •

edited

Loading

codecov bot commented Jun 23, 2023 •

edited

Loading