Skip to content

Conversation

@mdavis36
Copy link
Collaborator

No description provided.

@github-actions
Copy link

github-actions bot commented Oct 15, 2025

Review updated until commit e3b66c5

Description

  • Limit LLVM header inclusion to only required files

  • Add debug messages for LLVM configuration details

  • Apply LLVM includes directly to jit.cpp via source properties

  • Remove global SYSTEM include for better encapsulation


Changes walkthrough 📝

Relevant files
Bug fix
CMakeLists.txt
Scoped LLVM includes and enhanced build logging                   

CMakeLists.txt

  • Added verbose status messages for LLVM version and paths
  • Removed target_include_directories from LLVM_JIT INTERFACE
  • Applied -isystem flags directly to jit.cpp source file
  • Improved encapsulation by limiting LLVM include scope
  • +15/-1   

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🧪 No relevant tests
    ⚡ Recommended focus areas for review

    Missing Include Directories

    The removal of target_include_directories for the LLVM_JIT interface library may lead to missing include paths for other translation units that depend on LLVM headers, potentially causing compilation issues outside of jit.cpp.

    add_library(LLVM_JIT INTERFACE)
    target_compile_definitions(LLVM_JIT INTERFACE ${LLVM_DEFINITIONS})
    target_link_libraries(LLVM_JIT INTERFACE ${LLVM_LIBS})
    
    Compile Flag Handling

    Using COMPILE_FLAGS with -isystem directly in set_source_files_properties may not be portable across all CMake generators and could interfere with other compile options; a target-based approach would be more maintainable and consistent.

    set_source_files_properties(
      ${NVFUSER_SRCS_DIR}/host_ir/jit.cpp
      PROPERTIES
      COMPILE_FLAGS "-isystem ${LLVM_INCLUDE_DIRS}"
    )
    

    @mdavis36
    Copy link
    Collaborator Author

    !test

    1 similar comment
    @mdavis36
    Copy link
    Collaborator Author

    !test

    @mdavis36
    Copy link
    Collaborator Author

    !test

    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