[NDMatrix] Resolved Dangling Reference Warning

[AlexandreSinger]

GCC13 introduced a warning for potentially dangling references. The NDMatrix class was creating temporary objects to store the offseted pointer and then returning refrences to these pointers. Although this is correct, since the pointer is owned by the base NDMatrix object, it was confusing the compiler which is not something you want to do in general.

Changed how the NDMatrix proxy class works by storing the offset into the pointer directly and passing a reference to the base pointer instead. This makes it clear to the compiler that this pointer belongs to another class and we are returning a reference to a portion of that memory, which is allowed.

[AlexandreSinger]

@soheilshahrouz In case you were curious, this resolves the NDMatrix issues I was talking about. This should get us through the next few Ubuntu versions without issue. We may not even need to go to MDSpan.

[AlexandreSinger]

@vaughnbetz This PR fixes a whole mess of warnings from the Ubuntu 24.04 build. The issue was that the NDMatrix class was creating a temporary sub-matrix objects per dimension during accesses (each of which were given a pointer which was offset from the base pointer of the NDMatrix class). This caused the compiler to get confused about who owned the pointer and it thought that we were returning a reference to a temporary object in memory when we accessed this pointer.

To fix this I changed the NDMatrix class to pass the base pointer by reference and only had the offset get passed to each sub-matrix. This makes it obvious to the compiler that the pointer in the sub-matrix objects do not belong to them. This resolved the warnings and I think it may have a slightly positive affect on performance; I am running the VTR benchmarks now.

This will allow us to move to the next version of Ubuntu for the CI and remain warning clean. There are still a couple of warnings left, but this one was definitely the worst to fix. Please review when you have a moment.

[AlexandreSinger]

VTR Benchmarks results. Seems to be a wash in runtime; which is good.

	baseline_parse_results.txt	matrix_change_parse_results.txt
vtr_flow_elapsed_time	1	0.999288384
odin_synth_time
parmys_synth_time	1	0.995853649
abc_depth	1	1
abc_synth_time	1	0.996613896
num_clb	1	1
num_memories	1	1
num_mult	1	1
max_vpr_mem	1	1.000222248
num_pre_packed_blocks	1	1
num_post_packed_blocks	1	1
device_grid_tiles	1	1
pack_time	1	0.994544163
placed_wirelength_est	1	1
place_time	1	1.006528529
placed_CPD_est	1	1
min_chan_width	1	1
routed_wirelength	1	1
min_chan_width_route_time	1	0.995949371
crit_path_routed_wirelength	1	1
critical_path_delay	1	1
geomean_nonvirtual_intradomain_critical_path_delay	1	1
crit_path_route_time	1	0.992836336

The QoR did not change at all which is also good. Let me know what you think about this change @vaughnbetz

[vaughnbetz]

Looks good -- I suggest a few more comments though.

[vaughnbetz]

Comment these data members if you know what they are. Since offset was just added it at least should be commentable.

[vaughnbetz]

I assume going from a T* to a unique_ptr reference has some advantage, and if there is that would be a good thing to mention in the comment.

[AlexandreSinger]

Added a comment. The main benefit is safety. This makes it clear that the owner of the memory is someone else (the base NDMatrix object). This also allows the base pointer to be reallocated without invalidating all of the NDMatrixProxy objects (since they would be pointing to the stale data). This was the change that helped the compiler better understand what we were doing here. What we were doing was not wrong, just a bit hard to analyze.

[vaughnb-cerebras]

Looks great, thanks!