Skip to content

Updated generated filenames in run_command.sh#3805

Merged
jacobhinkle merged 2 commits intomainfrom
jh/fix_codediff_runcommand
Feb 4, 2025
Merged

Updated generated filenames in run_command.sh#3805
jacobhinkle merged 2 commits intomainfrom
jh/fix_codediff_runcommand

Conversation

@jacobhinkle
Copy link
Collaborator

In #3788, the filename pattern __tmp_kernel*.cu was updated to __tmp_*.cu in compare_codegen.sh. That fix was incomplete, because run_command.sh also needs to move the generated files. This fix implements that change. I think this should finally fix codediff.

In #3788, the filename pattern `__tmp_kernel*.cu` was updated to
`__tmp_*.cu` in `compare_codegen.sh`. That fix was incomplete, because
`run_command.sh` also needs to move the generated files. This fix
implements that change. I think this should finally fix codediff.
@jacobhinkle
Copy link
Collaborator Author

!test --diff

@github-actions
Copy link

github-actions bot commented Jan 31, 2025

Review updated until commit 15135e8

Description

  • Updated filename pattern in run_command.sh

  • Ensures consistency with compare_codegen.sh


Changes walkthrough 📝

Relevant files
Enhancement
run_command.sh
Update filename pattern in run_command.sh                               

tools/codediff/run_command.sh

  • Updated find command to use __tmp_*.cu and __tmp_*.ptx patterns
  • Applied changes in movecudafiles and cleanup functions
  • +4/-4     

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🧪 No relevant tests
    ⚡ Recommended focus areas for review

    Filename Pattern

    Ensure that the new filename pattern __tmp_*.cu and __tmp_*.ptx is consistent with other parts of the codebase and does not introduce any unintended side effects.

    find . -maxdepth 1 -name '__tmp_*.cu' -print0 | xargs -0 --no-run-if-empty mv -t "$1/cuda"
    find . -maxdepth 1 -name '__tmp_*.ptx' -print0 | xargs -0 --no-run-if-empty mv -t "$1/ptx"
    Cleanup Function

    Verify that the cleanup function correctly identifies and removes all temporary files matching the new pattern __tmp_*.cu and __tmp_*.ptx.

    cleanup() {
        numcu=$(find . -maxdepth 1 -name '__tmp_*.cu' | wc -l)
        numptx=$(find . -maxdepth 1 -name '__tmp_*.ptx' | wc -l)

    @jacobhinkle jacobhinkle requested a review from xwang233 January 31, 2025 21:04
    Copy link
    Collaborator

    @xwang233 xwang233 left a comment

    Choose a reason for hiding this comment

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

    I think it's ok to update the file name pattern matching for copy/paste/cleanup. We need to make sure the diff compare script is able to handle the broader name patterns.

    E.g.

    # find ptx file if it exists
    ptx_basename = os.path.splitext(basename)[0] + ".ptx"
    ptx_fullname = os.path.join(self.directory, "ptx", ptx_basename)

    iiuc, the .ptx file name without extension is expected to be the same as .cu file. Is this certain now, i.e. not having one __tmp_kernel (.cu) and one __tmp_nvfuser (.ptx, or vice versa), etc?

    @jacobhinkle
    Copy link
    Collaborator Author

    Is this certain now, i.e. not having one __tmp_kernel (.cu) and one __tmp_nvfuser (.ptx, or vice versa), etc?

    This should have been fixed by #3788

    @jacobhinkle
    Copy link
    Collaborator Author

    !test --diff

    @jacobhinkle
    Copy link
    Collaborator Author

    Codediff is now working following some changes to internal tools.

    @jacobhinkle jacobhinkle merged commit 4205803 into main Feb 4, 2025
    56 of 57 checks passed
    @jacobhinkle jacobhinkle deleted the jh/fix_codediff_runcommand branch February 4, 2025 14:47
    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