diff mbox series

[4/4] libgomp/oacc-mem: add missing assert to goacc_enter_datum

Message ID 20260505134929.3522938-5-aarsenovic@baylibre.com
State New
Headers
Series GCN: Target offload overhead improvements, batch 2 |

Commit Message

Arsen Arsenović May 5, 2026, 1:15 p.m. UTC 
  A bug I accidentally introduced made it so that new variables are
allocated with some room to spare before them, and ergo, that tgt_offset
!= 0, leading to tests failing in what looked like a strange way.  Turns
out, goacc_enter_datum was failing to validate its assumption that
tgt_offset == 0.  This patch adds that assert.

libgomp/ChangeLog:

	* oacc-mem.c (goacc_enter_datum): Assert that tgt_offset of the
	newly-mapped variable is zero.
---
 libgomp/oacc-mem.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Thomas Schwinge May 7, 2026, 9:48 a.m. UTC | #1 
Hi Arsen!

On 2026-05-05T15:15:00+0200, Arsen Arsenović <aarsenovic@baylibre.com> wrote:
> A bug I accidentally introduced made it so that new variables are
> allocated with some room to spare before them, and ergo, that tgt_offset
> != 0, leading to tests failing in what looked like a strange way.  Turns
> out, goacc_enter_datum was failing to validate its assumption that
> tgt_offset == 0.  This patch adds that assert.

Thanks!  That's OK to push already in front of your other changes.

> --- a/libgomp/oacc-mem.c
> +++ b/libgomp/oacc-mem.c
> @@ -593,6 +593,9 @@ goacc_enter_datum (void **hostaddrs, size_t *sizes, void *kinds, int async)
>        assert (n);
>        assert (n->refcount == 1);
>        assert (n->dynamic_refcount == 0);
> +      assert (/* This is only reached if we're mapping a new, singular
> +		 variable, and so, its offset ought to be zero.  */
> +	      n->tgt_offset == 0);

Not sure this comments adds more insight than already is provided by the
surrounding code -- but I'm not objecting: I rather have one comment too
much than one too little.  ;-)

>        n->dynamic_refcount++;
>  
>        d = (void *) tgt->tgt_start;

I did wonder if, despite the 'assert', we'd make this:

    d = (void *) tgt->tgt_start + n->tgt_offset;

..., to make clear the way this ought to be handled, but I guess there's
no reason to do that now, and it'll be obvious if we ever need to again
get rid of the 'assert' you're now introducing.


Grüße
 Thomas
diff mbox series

Patch

diff --git a/libgomp/oacc-mem.c b/libgomp/oacc-mem.c
index 5601daf13957..f9ceb094d156 100644
--- a/libgomp/oacc-mem.c
+++ b/libgomp/oacc-mem.c
@@ -593,6 +593,9 @@  goacc_enter_datum (void **hostaddrs, size_t *sizes, void *kinds, int async)
       assert (n);
       assert (n->refcount == 1);
       assert (n->dynamic_refcount == 0);
+      assert (/* This is only reached if we're mapping a new, singular
+		 variable, and so, its offset ought to be zero.  */
+	      n->tgt_offset == 0);
       n->dynamic_refcount++;
 
       d = (void *) tgt->tgt_start;