Skip to content

Expand boundary subview host coverage #10

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
psychocoderHPC wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Expand boundary subview host coverage #10

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 32 additions & 4 deletions include/alpaka/mem/View.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,11 +233,39 @@ namespace alpaka
constexpr auto getSubView(
alpaka::BoundaryDirection<View::dim(), LowHaloVecType, UpHaloVecType> boundaryDir) const
{
constexpr uint32_t dim = View::dim();
auto offset = alpaka::Vec<uint32_t, dim>{};
auto extents = alpaka::Vec<uint32_t, dim>{};
auto offset = typename T_Extents::UniVec{};
auto extents = typename T_Extents::UniVec{};

for(uint32_t i = 0; i < dim; ++i)
for(uint32_t i = 0; i < View::dim(); ++i)
{
switch(boundaryDir.data[i])
{
case BoundaryType::LOWER:
offset[i] = 0;
extents[i] = boundaryDir.lowerHaloSize[i];
break;
case BoundaryType::UPPER:
offset[i] = this->getExtents()[i] - boundaryDir.upperHaloSize[i];
extents[i] = boundaryDir.upperHaloSize[i];
break;
case BoundaryType::MIDDLE:
offset[i] = boundaryDir.lowerHaloSize[i];
extents[i] = this->getExtents()[i] - boundaryDir.lowerHaloSize[i] - boundaryDir.upperHaloSize[i];
break;
Comment on lines +247 to +254
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if T_Extents::UniVec typically uses signed or unsigned element types in this codebase
echo "=== Checking Vec element types in the codebase ==="
rg -n "allocHost<" --type cpp -A2 | head -40

echo ""
echo "=== Checking UniVec definition ==="
ast-grep --pattern 'using UniVec = $_'

Repository: psychocoderHPC/alpaka3

Length of output: 2340

🏁 Script executed:

# Read the View.hpp file around the specified lines
cat -n include/alpaka/mem/View.hpp | sed -n '230,295p'

Repository: psychocoderHPC/alpaka3

Length of output: 3478

🏁 Script executed:

# Search for T_Extents definition and BoundaryDir structure
echo "=== Looking for T_Extents usage and definition ==="
rg -n "T_Extents" include/alpaka/mem/View.hpp -B2 -A2 | head -60

echo ""
echo "=== Looking for BoundaryDir definition ==="
rg -n "struct BoundaryDir\|class BoundaryDir" include/alpaka/mem/View.hpp -A10

echo ""
echo "=== Looking for halo size member types ==="
rg -n "lowerHaloSize\|upperHaloSize" include/alpaka/mem/View.hpp -B2 -A1 | head -40

Repository: psychocoderHPC/alpaka3

Length of output: 3092

🏁 Script executed:

# Search for BoundaryDirection definition
echo "=== Looking for BoundaryDirection definition ==="
rg -n "struct BoundaryDirection\|class BoundaryDirection" --type cpp -A15

echo ""
echo "=== Looking for BoundaryType enum ==="
rg -n "enum.*BoundaryType\|BoundaryType::" --type cpp -B2 -A8 | head -50

Repository: psychocoderHPC/alpaka3

Length of output: 153

🏁 Script executed:

# Search for where BoundaryDirection is used or created to see what types are passed
echo "=== Looking for BoundaryDirection instantiation ==="
rg -n "BoundaryDirection<" --type cpp | head -20

Repository: psychocoderHPC/alpaka3

Length of output: 2166

🏁 Script executed:

# Find BoundaryDirection definition
echo "=== Looking for BoundaryDirection struct definition ==="
rg -n "struct BoundaryDirection" include/alpaka/mem/BoundaryIter.hpp -A25

echo ""
echo "=== Looking for BoundaryType enum definition ==="
rg -n "enum.*BoundaryType" include/alpaka/mem/ -r cpp -A15

Repository: psychocoderHPC/alpaka3

Length of output: 4346

🏁 Script executed:

# Check the actual implementation in test to see what types are used
cat -n test/unit/mem/subDataStorage.cpp | sed -n '240,320p'

Repository: psychocoderHPC/alpaka3

Length of output: 4040

🏁 Script executed:

# Check what T_Extents is typically in the View usage
echo "=== Checking View instantiation examples ==="
rg -n "View<" test/unit/mem/subDataStorage.cpp -B3 -A3 | head -50

echo ""
echo "=== Check View template parameters ==="
cat -n test/unit/mem/subDataStorage.cpp | sed -n '1,100p' | grep -A20 "SECTION"

Repository: psychocoderHPC/alpaka3

Length of output: 2581

🏁 Script executed:

# Get the buffer creation to understand T_Extents
cat -n test/unit/mem/subDataStorage.cpp | sed -n '200,245p'

Repository: psychocoderHPC/alpaka3

Length of output: 2233

🏁 Script executed:

# Check what happens with the degenerate case - there must be some validation or clamping
echo "=== Checking the getSubView(offset, extents) implementation ==="
cat -n include/alpaka/mem/View.hpp | sed -n '188,230p'

Repository: psychocoderHPC/alpaka3

Length of output: 2585

🏁 Script executed:

# Let's also check what Vec element type is used
echo "=== Checking how Vec is typically instantiated ==="
rg -n "Vec{" test/unit/mem/subDataStorage.cpp | head -20

Repository: psychocoderHPC/alpaka3

Length of output: 205

🏁 Script executed:

# Check how Vec is instantiated
echo "=== Checking Vec instantiation ==="
rg -n "alpaka::Vec\{" test/unit/mem/subDataStorage.cpp | head -20

Repository: psychocoderHPC/alpaka3

Length of output: 1768

🏁 Script executed:

# Understand what type Vec uses - check the actual variables in test
cat -n test/unit/mem/subDataStorage.cpp | sed -n '232,250p'

Repository: psychocoderHPC/alpaka3

Length of output: 842

🏁 Script executed:

# Check the actual T_Extents type used in allocHost
echo "=== Looking at allocHost and buffer getView ==="
rg -n "allocHost" include/alpaka/ -A5 | grep -A5 "def\|return" | head -40

echo ""
echo "=== Check what type T_Extents is typically ==="
rg -n "using T_Extents\|T_Extents =" include/alpaka/mem/View.hpp

Repository: psychocoderHPC/alpaka3

Length of output: 860

🏁 Script executed:

# Let's trace through the test more carefully - what is the buffer type and therefore View's T_Extents?
cat -n test/unit/mem/subDataStorage.cpp | sed -n '1,50p'

Repository: psychocoderHPC/alpaka3

Length of output: 1868

🏁 Script executed:

# Let's check the allocHost implementation to determine what T_Extents type becomes
echo "=== Check allocHost return type and buffer View ==="
rg -n "allocHost" include/alpaka/onHost/interface.hpp -B2 -A10 | head -50

echo ""
echo "=== Check what makeView creates ==="
rg -n "makeView" include/alpaka/mem/ -A5 | head -40

Repository: psychocoderHPC/alpaka3

Length of output: 3623

🏁 Script executed:

# Look at the actual test case with degenerate halos to understand what types are involved
cat -n test/unit/mem/subDataStorage.cpp | sed -n '310,320p'

Repository: psychocoderHPC/alpaka3

Length of output: 619

🏁 Script executed:

# Check if there's any validation or documentation about halo sizes
echo "=== Check for preconditions or validation on halo sizes ==="
rg -n "halo" include/alpaka/mem/View.hpp -B2 -A2

echo ""
echo "=== Check the BoundaryDirection documentation ==="
rg -n "BoundaryDirection" include/alpaka/mem/BoundaryIter.hpp -B5 | head -50

Repository: psychocoderHPC/alpaka3

Length of output: 2196

🏁 Script executed:

# Key question: what type is T_Type when allocHost is called with int?
# Let's trace through the test to see what T_Extents actually is
echo "=== Check allocHost<int> and what T_Extents becomes ==="
cat -n test/unit/mem/subDataStorage.cpp | sed -n '15,25p'

echo ""
echo "=== Check the Vec type deduction ==="
rg -n "Vec const extentsVec" include/alpaka/onHost/interface.hpp -B5 -A5

Repository: psychocoderHPC/alpaka3

Length of output: 927

🏁 Script executed:

# I need to understand what T_Extents actually resolves to
# Let's check how the buffer is created and what type T_Extents becomes
echo "=== Full trace: allocHost creates a buffer with what T_Extents? ==="
cat -n include/alpaka/mem/View.hpp | sed -n '37,70p'

echo ""
echo "=== Check ALPAKA_TYPEOF to understand type deduction ==="
rg -n "ALPAKA_TYPEOF" include/alpaka/ -A2 | head -30

Repository: psychocoderHPC/alpaka3

Length of output: 3644

🏁 Script executed:

# The critical insight: what type is Vec{4, 5, 6} vs Vec{1u, 2u, 3u}?
echo "=== Test with degenerate case - let's verify the actual problem scenario ==="
cat -n test/unit/mem/subDataStorage.cpp | sed -n '311,317p'

echo ""
echo "=== Check if there's input validation anywhere ==="
rg -n "lowerHaloSize\|upperHaloSize" include/alpaka/mem/BoundaryIter.hpp -B3 -A3

Repository: psychocoderHPC/alpaka3

Length of output: 632

🏁 Script executed:

# Check for any precondition or documentation about valid halo sizes
echo "=== Check getSubView documentation ==="
cat -n include/alpaka/mem/View.hpp | sed -n '189,202p'

echo ""
echo "=== Check if BoundaryDirection has any constraints ==="
cat -n include/alpaka/mem/BoundaryIter.hpp | sed -n '32,100p'

Repository: psychocoderHPC/alpaka3

Length of output: 4112

🏁 Script executed:

# Verify what actually happens with invalid halos - does test cover the underflow case?
echo "=== Search for any other halo validation or comments ==="
rg -n "halo.*valid\|valid.*halo\|precondition\|underflow" include/alpaka/mem/ --type cpp

Repository: psychocoderHPC/alpaka3

Length of output: 122

🏁 Script executed:

# One final check - does the code path handle the case where halos exceed extents?
# The test has a degenerate case where halos consume dimensions, but what about exceeding?
echo "=== Check if there are any tests with halos exceeding extents ==="
rg -n "allocHost\|makeCoreBoundaryDirection" test/unit/mem/subDataStorage.cpp -A5 | head -60

echo ""
echo "=== Check if extents and offset/extents arithmetic can be at compile-time ==="
cat -n include/alpaka/mem/View.hpp | sed -n '232,260p'

Repository: psychocoderHPC/alpaka3

Length of output: 1721

Unsigned integer underflow with invalid halo sizes is mitigated by downstream bounds checking, but preconditions should be documented.

If T_Extents::UniVec::type is unsigned (e.g., uint32_t) and halo sizes exceed the view extent, the subtractions at lines 248/277 (UPPER offset) and 253/282 (MIDDLE extent) will wrap around to large positive values.

However, the downstream assert((offsetMd + extentMd <= this->getExtents()).reduce(std::logical_and{})) in getSubView(offset, extents) at line 211 catches these cases at runtime: the underflowed offset plus extents will exceed this->getExtents(), failing the assertion.

The issue is that this function is constexpr, and asserts may not be enforced at compile-time, potentially allowing invalid halo sizes to create undefined behavior if evaluated during constant evaluation. Consider adding explicit precondition validation (e.g., checks that lowerHaloSize + upperHaloSize <= extent for MIDDLE cases) or documenting the requirement that halo sizes must not exceed extents.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@include/alpaka/mem/View.hpp` around lines 247 - 254, The subtraction of halo
sizes from extents in the BoundaryType handling (using this->getExtents(),
offset, extents) can underflow for unsigned extent types and is only caught
later by getSubView(offset, extents) asserts; add explicit constexpr
precondition checks in the same function to prevent unsigned wraparound:
validate for BoundaryType::UPPER that boundaryDir.upperHaloSize[i] <=
this->getExtents()[i], for BoundaryType::MIDDLE that
boundaryDir.lowerHaloSize[i] + boundaryDir.upperHaloSize[i] <=
this->getExtents()[i], and similarly ensure lower halo <= extent for LOWER cases
(or document the precondition) so invalid halo sizes are rejected at
compile-time or immediately at runtime before performing the subtraction.

default:
throw std::invalid_argument("invalid direction");
}
}
return getSubView(offset, extents);
}

template<alpaka::concepts::Vector LowHaloVecType, alpaka::concepts::Vector UpHaloVecType>
constexpr auto getSubView(alpaka::BoundaryDirection<View::dim(), LowHaloVecType, UpHaloVecType> boundaryDir)
{
auto offset = typename T_Extents::UniVec{};
auto extents = typename T_Extents::UniVec{};

for(uint32_t i = 0; i < View::dim(); ++i)
{
switch(boundaryDir.data[i])
{
Expand Down
148 changes: 148 additions & 0 deletions test/unit/mem/subDataStorage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,3 +118,151 @@ TEST_CASE("3D alpaka::View::getSubView function tests", "[mem][view][SubDataStor
}
}
}

TEST_CASE(
"alpaka::View::getSubView(BoundaryDirection) covers host lower upper core and const aliasing",
"[mem][view][SubDataStorage]")
{
alpaka::Vec totalExtents{4, 5, 6};
auto buffer0 = alpaka::onHost::allocHost<int>(totalExtents);

for(auto idx : alpaka::IdxRange{totalExtents})
{
buffer0[idx] = idx.z() * 100 + idx.y() * 10 + idx.x();
}

auto view0 = buffer0.getView();
auto const lowerHalos = alpaka::Vec{1u, 2u, 1u};
auto const upperHalos = alpaka::Vec{2u, 1u, 3u};
using HaloVec = std::remove_cvref_t<decltype(lowerHalos)>;
auto const makeBoundary = [&](auto const& boundaries)
{
return alpaka::BoundaryDirection<3, HaloVec, HaloVec>{
boundaries,
lowerHalos,
upperHalos};
};

SECTION("lower boundary keeps the origin region")
{
// LOWER should select the leading halo without shifting the origin in any chosen dimension.
auto subView = view0.getSubView(
makeBoundary(alpaka::Vec{alpaka::BoundaryType::LOWER, alpaka::BoundaryType::MIDDLE, alpaka::BoundaryType::LOWER}));

auto const expectedExtents = alpaka::Vec{1, 2, 1};
auto const expectedOffset = alpaka::Vec{0, static_cast<int>(lowerHalos.y()), 0};

REQUIRE(subView.getExtents() == expectedExtents);

for(auto idx : alpaka::IdxRange{expectedExtents})
{
REQUIRE(subView[idx] == view0[expectedOffset + idx]);
}
}

SECTION("upper boundary maps to the shifted tail region")
{
// UPPER should start from the tail extents minus the upper halo sizes.
auto subView = view0.getSubView(
makeBoundary(alpaka::Vec{alpaka::BoundaryType::UPPER, alpaka::BoundaryType::UPPER, alpaka::BoundaryType::MIDDLE}));

auto const expectedExtents = alpaka::Vec{2, 1, 2};
auto const expectedOffset = alpaka::Vec{
totalExtents.z() - static_cast<int>(upperHalos.z()),
totalExtents.y() - static_cast<int>(upperHalos.y()),
static_cast<int>(lowerHalos.x())};

REQUIRE(subView.getExtents() == expectedExtents);

for(auto idx : alpaka::IdxRange{expectedExtents})
{
REQUIRE(subView[idx] == view0[expectedOffset + idx]);
}
}

SECTION("middle boundary returns the asymmetric interior")
{
// MIDDLE should crop by the lower and upper halos independently in each dimension.
auto subView = view0.getSubView(alpaka::makeCoreBoundaryDirection<3>(lowerHalos, upperHalos));

auto const expectedOffset = alpaka::Vec{
static_cast<int>(lowerHalos.z()),
static_cast<int>(lowerHalos.y()),
static_cast<int>(lowerHalos.x())};
auto const expectedExtents = alpaka::Vec{
totalExtents.z() - static_cast<int>(lowerHalos.z()) - static_cast<int>(upperHalos.z()),
totalExtents.y() - static_cast<int>(lowerHalos.y()) - static_cast<int>(upperHalos.y()),
totalExtents.x() - static_cast<int>(lowerHalos.x()) - static_cast<int>(upperHalos.x())};

REQUIRE(subView.getExtents() == expectedExtents);

for(auto idx : alpaka::IdxRange{expectedExtents})
{
REQUIRE(subView[idx] == view0[expectedOffset + idx]);
}
}

SECTION("degenerate middle boundary with a consumed dimension stays empty")
{
// If halos consume a dimension completely, the middle region should be a valid zero-extent subview.
auto subView = view0.getSubView(alpaka::makeCoreBoundaryDirection<3>(alpaka::Vec{1u, 2u, 3u}, alpaka::Vec{2u, 3u, 3u}));

REQUIRE(subView.getExtents() == alpaka::Vec{1, 0, 0});

auto count = 0;
for([[maybe_unused]] auto&& value : subView)
{
alpaka::unused(value);
++count;
}
REQUIRE(count == 0);
}

SECTION("const boundary subviews stay read-only and still read shifted values")
{
// The boundary-direction overload must propagate constness just like the offset overload does.
auto const& constView0 = view0;
auto subView = constView0.getSubView(
makeBoundary(alpaka::Vec{alpaka::BoundaryType::UPPER, alpaka::BoundaryType::MIDDLE, alpaka::BoundaryType::UPPER}));

auto const expectedOffset = alpaka::Vec{
totalExtents.z() - static_cast<int>(upperHalos.z()),
static_cast<int>(lowerHalos.y()),
totalExtents.x() - static_cast<int>(upperHalos.x())};
auto const expectedExtents = alpaka::Vec{2, 2, 3};

REQUIRE(subView.getExtents() == expectedExtents);
static_assert(std::is_const_v<std::remove_pointer_t<decltype(subView.data())>>);
static_assert(std::is_const_v<std::remove_reference_t<decltype(subView[alpaka::Vec{0, 0, 0}])>>);

for(auto idx : alpaka::IdxRange{expectedExtents})
{
REQUIRE(subView[idx] == view0[expectedOffset + idx]);
}
}

SECTION("non-const boundary subviews preserve mutability and alias the parent")
{
// Non-const parents should keep writable element access so boundary views can update the original storage.
auto subView = view0.getSubView(
makeBoundary(alpaka::Vec{alpaka::BoundaryType::LOWER, alpaka::BoundaryType::UPPER, alpaka::BoundaryType::MIDDLE}));

auto const expectedOffset
= alpaka::Vec{0, totalExtents.y() - static_cast<int>(upperHalos.y()), static_cast<int>(lowerHalos.x())};
auto const expectedExtents = alpaka::Vec{1, 1, 2};

REQUIRE(subView.getExtents() == expectedExtents);
static_assert(!std::is_const_v<std::remove_pointer_t<decltype(subView.data())>>);
static_assert(!std::is_const_v<std::remove_reference_t<decltype(subView[alpaka::Vec{0, 0, 0}])>>);

for(auto idx : alpaka::IdxRange{expectedExtents})
{
subView[idx] = 700 + static_cast<int>(alpaka::linearize(expectedExtents, idx));
}

for(auto idx : alpaka::IdxRange{expectedExtents})
{
REQUIRE(view0[expectedOffset + idx] == 700 + static_cast<int>(alpaka::linearize(expectedExtents, idx)));
}
}
}
Loading