Skip to content

Conversation

@boomanaiden154
Copy link
Contributor

@boomanaiden154 boomanaiden154 commented Oct 12, 2025

The overload accepting a llvm::StringRef is deprecated and will be removed once LLVM 22 branches.

Description

Before your PR is "Ready for review"

  • All commits are signed-off to indicate that your contribution adheres to the Developer Certificate of Origin requirements
  • Necessary tests have been added
  • Documentation is up-to-date
  • Auto-generated files modified by compiling Warp and building the documentation have been updated (e.g. __init__.pyi, functions.rst)
  • Code passes formatting and linting checks with pre-commit run -a

Summary by CodeRabbit

  • Bug Fixes

    • Restores successful C++ and CUDA compilation with LLVM 22+, resolving target detection and code generation issues seen on newer toolchains.
    • Maintains compatibility across older LLVM versions to prevent regressions.
  • Chores

    • Updated compiler compatibility logic to support LLVM 22+ while preserving behavior on earlier versions.

The overload accepting a llvm::StringRef is deprecated and will be
removed once LLVM 22 branches.

Signed-off-by: Aiden Grossman <[email protected]>
@coderabbitai
Copy link

coderabbitai bot commented Oct 12, 2025

📝 Walkthrough

Walkthrough

Adds LLVM_VERSION_MAJOR conditionals in warp/native/clang/clang.cpp to use llvm::Triple-aware APIs for target lookup and TargetMachine creation when building against LLVM 22+, while retaining existing string-based calls for older LLVM versions in both C++ and CUDA compilation paths.

Changes

Cohort / File(s) Summary
LLVM 22+ compatibility guards
warp/native/clang/clang.cpp
Wrapped target lookup and TargetMachine creation in LLVM_VERSION_MAJOR checks. For LLVM >= 22, uses llvm::TargetRegistry::lookupTarget with llvm::Triple(...) and creates TargetMachine with llvm::Triple(...). For older LLVM, retains string-based APIs. Applied in wp_compile_cpp and wp_compile_cuda.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant WP as wp_compile_cpp/wp_compile_cuda
  participant TR as llvm::TargetRegistry
  participant TM as llvm::TargetMachine

  rect rgba(220,235,255,0.4)
  note over WP: Determine LLVM version at compile-time
  alt LLVM >= 22
    WP->>TR: lookupTarget(llvm::Triple(target), error)
    TR-->>WP: Target*
    WP->>TM: createTargetMachine(Triple, CPU, Features, Options,...)
    TM-->>WP: TargetMachine*
  else LLVM < 22
    WP->>TR: lookupTarget(target_string, error)
    TR-->>WP: Target*
    WP->>TM: createTargetMachine(target_string, CPU, Features, Options,...)
    TM-->>WP: TargetMachine*
  end
  note over WP,TM: Use data layout and emit code as before
  end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly describes the primary change of switching to the lookupTarget overload that accepts an llvm::Triple, accurately reflecting the main update introduced in this pull request without extraneous detail.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 86373e3 and d920e14.

📒 Files selected for processing (1)
  • warp/native/clang/clang.cpp (2 hunks)

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@shi-eric shi-eric merged commit 6798a39 into NVIDIA:main Oct 17, 2025
3 checks passed
@boomanaiden154 boomanaiden154 deleted the lookup-target-string-triple branch October 17, 2025 19:32
@shi-eric
Copy link
Contributor

Thanks @boomanaiden154, this is merged.

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