FSI-SPH: fix the handling of markers outside the computational domain - #792
Conversation
|
I only read the description, but didn't get a chance to look at the code yet. Periodic boundary conditions were not revisited in detail after the couple of major redesigns of Chrono::FSI in the past year. No surprise there are lurking issues. One thing that is puzzling and worthy of more investigation is the failure of |
Three coupled defects, plus a runtime check on the invariant the first fix establishes. They are fixed together because correcting the guard of item 2 is what lets markers reach the hashing code along x and y, so fixing it alone would widen items 1 and 3 from one axis to three. 1. Out-of-bounds device write. calcGridHash adjusted an out-of-range bin index by a SINGLE grid period, so a marker more than one period outside a periodic axis kept an out-of-range index and the returned hash exceeded numCells. findCellStartEndD uses that hash to index arrays sized numCells, so the write went out of bounds, silently: no fault, no diagnostic, exit 0. An in-kernel diagnostic counted 165 to 375 out-of-range hashes per configuration for a body outside a 0.1 m periodic domain. The reduction is now total: periodic axes wrap, non-periodic axes clamp to the edge bin, and within one period of the domain the result is unchanged. 2. Spurious termination. In calcHashD, "p.x < c.x || p.y < c.y || p.z < c.z && IsFluidParticle(...)" applied the rigid-body exemption to the z comparison alone, since && binds tighter than ||, so a body whose BCE markers left the domain along x or y terminated the run. That defeats the intent of a191fe7, "Fix that allows rigid bodies to be initialized and travel across computational domain boundaries". Both checks are now parenthesized. 3. Phantom forces, and markers that should interact but do not. Modify_Local_PosB is the whole body of Distance, so it is the distance used by 30 call sites. It applied the minimum-image shift on every axis without consulting x_periodic, y_periodic or z_periodic, and applied it at most ONCE. On a NON-periodic axis that fabricates an interaction between markers a box length apart, which item 1's reduction makes reachable by placing an outside marker in the edge bin. On a PERIODIC axis a marker more than one period out was binned into its correct image cell yet measured a whole box away, so it silently failed to interact with fluid it was sitting next to; rigid-body markers reach that state because ApplyPeriodicBoundary*_D deliberately does not wrap them. The shift now consults the flags and is total, so it agrees with the bin reduction of item 1. This also repairs utest_FSI-SPH_Poiseuille_flow, which fails on ROCm/HIP in the default single precision before it: 0 of 3 checks at a final density error of 2.27234e-04, against 3 of 3 and 8.42285e-06 after. It passes on CUDA and in double precision both before and after, so the manifestation is specific to HIP in single precision; why has NOT been established. calcHashD now also raises the existing error flag if a hash it computed falls outside the grid. The reduction makes that impossible by construction, so it should never fire; it is there because the failure it guards against is otherwise silent. Cost, measured because this touches the module's hottest device function, and it cuts both ways. Where the division never executes, on a 106k-particle non-periodic settling case, this is at worst neutral against unpatched and 2.5% faster than the single-period variant it supersedes. Where it does execute, on the fully periodic Poiseuille channel, it is slower: per unit of simulated time 2.6% slower than that variant and 3.9% slower than unpatched, with the two populations disjoint in all 49 pairs in both comparisons. Both figures come from 7 interleaved repeats per side. Normalizing by simulated time is necessary on that rung because unpatched does not finish the regression, and it carries the caveat that the unpatched solution is diverging while the other two are not. A fully periodic domain is the worst case for this change, since all three axes then take the division path. New unit test utest_FSI-SPH_domain_guard, five cases, four of which fail on unfixed code: the rigid-body exemption; that fluid outside is still reported, so the exemption cannot be mistaken for removing the guard; periodic equivalence of a body one and ten periods out; that a non-periodic body beyond kernel support leaves the fluid identical to a run with no body; and mixed periodicity, one periodic axis and two non-periodic ones at once, which is the common production case and which neither uniform case covers. Item 1 has no black-box test, which is measured rather than assumed: with the reduction left un-total, and the diagnostic confirming hundreds of out-of-range hashes, the periodic case still passes at zero deviation, because the out-of-bounds writes do not perturb observable output. Its evidence is the diagnostic and the runtime check. The out-of-line helper's noinline attribute is spelled per compiler behind a macro, because this device header is also compiled by the host compiler (ChFsiFluidSystemSPH.cpp includes it) and every single spelling is wrong somewhere: bare __noinline__ does not compile in a host translation unit, __attribute__((noinline)) without inline does not link, and MSVC rejects GNU attribute syntax. Validated on MI350X (gfx950, ROCm 7.2, Release) in both precisions and on CUDA in single precision (RTX 5090, nvcc 12.9, host g++ 13.3), alongside the module's existing unit tests and a 106k-particle settling regression, and in both cases with the new test shown to abort against an unpatched library built in the same worktree. Not validated, and neither is silent: no Windows build of Chrono was made, and CUDA in double precision cannot currently be run at all, for reasons that predate this change.
bd37f03 to
bd0a5a3
Compare
|
I'll look into the single precision part next week, leaving town in a couple of minutes. One thing already narrowed: the cause is the minimum-image shift (item 3), not the hashing One more thing: the description above says nvcc 12.0, and the toolkit actually used was nvcc 12.9 |
|
Followed up on the single-precision HIP failure, since it turned out to be quick. It is not the arithmetic. I rebuilt with one item reverted at a time: only the minimum-image change That variant and the old code compute the same thing, and in this test neither of the two things the So the thing worth knowing is about the test rather than the fix: Build: ROCm 7.2 on MI350X, -O3, no -ffast-math, --offload-arch=gfx950. |
2 participants
Three coupled defects, plus a runtime check on the invariant the first fix establishes. They are fixed together because correcting the guard of item 2 is what lets markers reach the hashing code along x and y, so fixing it alone would widen items 1 and 3 from one axis to three.
Out-of-bounds device write. calcGridHash adjusted an out-of-range bin index by a SINGLE grid period, so a marker more than one period outside a periodic axis kept an out-of-range index and the returned hash exceeded numCells. findCellStartEndD uses that hash to index arrays sized numCells, so the write went out of bounds, silently: no fault, no diagnostic, exit 0. An in-kernel diagnostic counted 165 to 375 out-of-range hashes per configuration for a body outside a 0.1 m periodic domain. The reduction is now total: periodic axes wrap, non-periodic axes clamp to the edge bin, and within one period of the domain the result is unchanged.
Spurious termination. In calcHashD, "p.x < c.x || p.y < c.y || p.z < c.z && IsFluidParticle(...)" applied the rigid-body exemption to the z comparison alone, since && binds tighter than ||, so a body whose BCE markers left the domain along x or y terminated the run. That defeats the intent of a191fe7, "Fix that allows rigid bodies to be initialized and travel across computational domain boundaries". Both checks are now parenthesized.
Phantom forces, and markers that should interact but do not. Modify_Local_PosB is the whole body of Distance, so it is the distance used by 30 call sites. It applied the minimum-image shift on every axis without consulting x_periodic, y_periodic or z_periodic, and applied it at most ONCE. On a NON-periodic axis that fabricates an interaction between markers a box length apart, which item 1's reduction makes reachable by placing an outside marker in the edge bin. On a PERIODIC axis a marker more than one period out was binned into its correct image cell yet measured a whole box away, so it silently failed to interact with fluid it was sitting next to; rigid-body markers reach that state because ApplyPeriodicBoundary*_D deliberately does not wrap them. The shift now consults the flags and is total, so it agrees with the bin reduction of item 1.
This also repairs utest_FSI-SPH_Poiseuille_flow, which fails on ROCm/HIP in the default single precision before it: 0 of 3 checks at a final density error of 2.27234e-04, against 3 of 3 and 8.42285e-06 after. It passes on CUDA and in double precision both before and after, so the manifestation is specific to HIP in single precision; why has NOT been established.
calcHashD now also raises the existing error flag if a hash it computed falls outside the grid. The reduction makes that impossible by construction, so it should never fire; it is there because the failure it guards against is otherwise silent.
Cost, measured because this touches the module's hottest device function, and it cuts both ways. Where the division never executes, on a 106k-particle non-periodic settling case, this is at worst neutral against unpatched and 2.5% faster than the single-period variant it supersedes. Where it does execute, on the fully periodic Poiseuille channel, it is slower: per unit of simulated time 2.6% slower than that variant and 3.9% slower than unpatched, with the two populations disjoint in all 49 pairs in both comparisons. Both figures come from 7 interleaved repeats per side. Normalizing by simulated time is necessary on that rung because unpatched does not finish the regression, and it carries the caveat that the unpatched solution is diverging while the other two are not. A fully periodic domain is the worst case for this change, since all three axes then take the division path.
New unit test utest_FSI-SPH_domain_guard, five cases, four of which fail on unfixed code: the rigid-body exemption; that fluid outside is still reported, so the exemption cannot be mistaken for removing the guard; periodic equivalence of a body one and ten periods out; that a non-periodic body beyond kernel support leaves the fluid identical to a run with no body; and mixed periodicity, one periodic axis and two non-periodic ones at once, which is the common production case and which neither uniform case covers. Item 1 has no black-box test, which is measured rather than assumed: with the reduction left un-total, and the diagnostic confirming hundreds of out-of-range hashes, the periodic case still passes at zero deviation, because the out-of-bounds writes do not perturb observable output. Its evidence is the diagnostic and the runtime check.
The out-of-line helper's noinline attribute is spelled per compiler behind a macro, because this device header is also compiled by the host compiler (ChFsiFluidSystemSPH.cpp includes it) and every single spelling is wrong somewhere: bare noinline does not compile in a host translation unit, attribute((noinline)) without inline does not link, and MSVC rejects GNU attribute syntax.
Validated on MI350X (gfx950, ROCm 7.2, Release) in both precisions and on CUDA in single precision (RTX 5090, nvcc 12.0), alongside the module's existing unit tests and a 106k-particle settling regression, and in both cases with the new test shown to abort against an unpatched library built in the same worktree. Not validated, and neither is silent: no Windows build of Chrono was made, and CUDA in double precision cannot currently be run at all, for reasons that predate this change.