Skip to content

Conversation

@scal444
Copy link
Collaborator

@scal444 scal444 commented Dec 5, 2025

We'd migrated gradients and other shared buffers but were still using global arrays for positions.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 5, 2025

Greptile Overview

Greptile Summary

Refactored per-molecule kernel implementations to use shared memory position buffers instead of global arrays. The device functions (molEnergy, molGrad, etc.) now receive pre-offset molecule-local coordinate pointers rather than performing internal offsetting from global arrays.

Key improvements:

  • Eliminated unnecessary global memory copies during BFGS line search iterations
  • Consistent shared memory usage across energy and gradient calculations
  • Device functions simplified by removing redundant pointer arithmetic
  • Added proper bfgs library dependency in CMakeLists.txt

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The refactoring is straightforward and maintains correctness by consistently passing molecule-local coordinate buffers. The changes improve memory access patterns without altering computational logic. The CMakeLists.txt change resolves a missing dependency.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
src/minimizer/bfgs_minimize_permol_kernels.cu 5/5 Updated to use local position buffers (shared memory) instead of global arrays, removing unnecessary global memory copies during line search
src/forcefields/dist_geom_kernels.cu 5/5 Refactored to pass pre-offset molecule coordinates to device functions for better shared memory utilization
src/forcefields/dist_geom_kernels_device.cuh 5/5 Changed function signatures to accept pre-offset molecule coordinates instead of global arrays with internal offsetting
src/forcefields/mmff_kernels.cu 5/5 Updated kernel calls to pass molecule-local coordinate pointers to device functions
src/forcefields/mmff_kernels_device.cuh 5/5 Modified device function signatures to receive pre-offset coordinate pointers for shared memory optimization
src/CMakeLists.txt 5/5 Added bfgs as PUBLIC dependency to etkdg library for proper linking

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Additional Comments (1)

  1. src/minimizer/bfgs_minimize_permol_kernels.cu, line 617-621 (link)

    style: unnecessary copy to global memory during line search - energy functions now read from scratchPos so this write has no effect until position is accepted (line 661)

6 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

6 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Copy link
Collaborator

@stslxg-nv stslxg-nv left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants