[comgr] enable multithreaded test#199216
Closed
accauble wants to merge 10000 commits into
Closed
Conversation
changes for comgr: amd/comgr/src/comgr-compiler.cpp amd/comgr/src/comgr-disassembly.cpp amd/comgr/src/comgr-hotswap-llvm.cpp 2 lit tests xfailed: jacob llvm/test/Bitcode/DILocalVariable-address-space.ll krishna mlir/test/Dialect/OpenMP/invalid.mlir
llvm#185377)" This reverts commit aeb14f7.
… intrinsic (llvm#185377)"" This reverts commit 9e72d74.
…rd TIMEF intrinsic (llvm#185377)" This reverts commit fda0f1c.
## Summary Bump Comgr version to 3.2 to match the hotswap public API (`amd_comgr_hotswap_rewrite`) that already shipped on `amd-staging`. ## Background - The hotswap rewrite API is tagged `AMD_COMGR_VERSION_3_2` in `amd/comgr/include/amd_comgr.h.in`. - `amd/comgr/src/exportmap.in` already exposes `amd_comgr_hotswap_rewrite` under the `amd_comgr_3.2` symbol version. - `amd/comgr/VERSION.txt` was never bumped past 3.0, so every nightly was producing `libamd_comgr.so.3.0.0` despite the >3.0 API surface. - `amd/comgr/src/amdcomgr.def` (Windows DLL exports) was missing `amd_comgr_hotswap_rewrite`, so the symbol was not exported on Windows builds. ## Changes - `amd/comgr/VERSION.txt`: `MINOR 0 → 2` - `amd/comgr/src/amdcomgr.def`: add `amd_comgr_hotswap_rewrite` ## ABI impact `SOVERSION` is `MAJOR`-only (`libamd_comgr.so.3`), so this bump is ABI-compatible — no consumer relinking required. The full versioned filename changes from `libamd_comgr.so.3.0.0` to `libamd_comgr.so.3.2.0`. --------- Co-authored-by: Claude <noreply@anthropic.com>
…th (#2386) ## Summary When HIP code is compiled to SPIR-V through COMGR (e.g. via hipRTC), the HIPAMD clang driver invokes `llvm-spirv` with flags like `--spirv-ext=+all,-SPV_KHR_untyped_pointers`. COMGR intercepts the command and runs it in-process via `writeSpirv`, but `executeSPIRVTranslator` only scanned `Args` for the input/output filenames and built `SPIRV::TranslatorOpts` from scratch with `enableAllExtensions()` — re-enabling `SPV_KHR_untyped_pointers` after the driver explicitly disabled it, producing SPIR-V with untyped pointers and breaking downstream BC link. This PR parses the `--spirv-*` subset emitted by HIPAMD (max-version, ext map with `+all`/`-EXT` semantics, debug-info-version, allow-unknown-intrinsics, preserve-auxdata) and configures `TranslatorOpts` to match, so the in-process path behaves like the standalone `amd-llvm-spirv` tool. --------- Co-authored-by: Claude Opus 4 (1M context) <noreply@anthropic.com>
## Motivation `amd/comgr/test-unit/CMakeLists.txt` discovers gtest from two places, both coupling Comgr's tests to the LLVM build: the in-tree `llvm_gtest` target, or an LLVM install built with `-DLLVM_INSTALL_GTEST=ON`. In TheRock superproject builds, `amd-llvm` is built without `LLVM_INSTALL_GTEST` (see ROCm/TheRock#4627), but TheRock ships a first-class `therock-googletest` subproject exposing the standard `GTest::` imported targets via `find_package(GTest)`. With the current logic, test-unit silently `return()`s with a warning. ## Technical Details Insert a middle tier into the discovery ladder: 1. `TARGET llvm_gtest` — unchanged. 2. **`find_package(GTest CONFIG QUIET)`** — new. Picks up vanilla googletest from superproject builds. 3. LLVM-installed `llvm_gtest` via `find_library` — unchanged. `comgr_match_llvm_rtti()` is unchanged; it still gates on `LLVM_ENABLE_RTTI` regardless of gtest source. ## Test Plan - LLVM monorepo build: still hits the `llvm_gtest` branch. - `-DLLVM_INSTALL_GTEST=ON` install: still hits the legacy fallback. - TheRock with `BUILD_DEPS therock-googletest` on `amd-comgr`: now builds via the new `find_package(GTest)` branch. Co-authored-by: Claude <noreply@anthropic.com>
llvm#185377)" This reverts commit aeb14f7.
ECTrans fixes: - Emit attach maps for use_device_* - Move attach maps to always be inserted into the map clause, regardless of which clause the main map is from - Remove optimization of skipping emission of unrequired attach maps, causes symmetry issues with stack size and breaks ectrans which depends on the stack layout being precise due to sketchy map choices ICON fixes: - Now that OpenMP has the attach map style, it's possible for pointer maps to bypass the size 0 check that PTR_AND_OBJ does before sending out data submits. This PR tries to add a size 0 barrier from submitting data to device for target update cases, the enter/exit map scenarios and regular target take different paths that already avoid this scenario. The size 0 submission is bad for multi-SDMA engine devices as we disallow these types of mappings and return an error in these cases. So, we need to avoid the submission to the plugin in the first place without impacting any ref counting or other runtime book keeping.
…2359) A predicate guarding a builtin with no required features is encoded as `@llvm.amdgcn.has.` with an empty suffix which is not handled properly Fixes ROCM-23825 --------- Co-authored-by: Jacob Lambert <jacob.lambert@amd.com>
… -- DS Instructions (#2369) Implements the `ds_*_2addr_stride64_*` trampoline patch for the GFX1250 B0-to-A0 HotSwap pipeline. This B0 erratum requires splitting one 8-byte DS 2-address instruction into two single-address DS instructions — a replacement larger than the original, necessitating trampoline emission. Each split correctly scales stride64 offsets, decomposes register pairs into their sub-registers, and bumps the next `s_wait_dscnt` immediate (via `MCCodeEmitter` re-encode) to account for the additional outstanding DS operation. When multiple DS2 sites precede the same wait instruction, bumps stack correctly (N+K, not N+1). The replacement sequence is placed in a nearby NOP sled when one is reachable, otherwise falls back to a trampoline. All operand extraction uses direct `MCInst` access — no string parsing of printed assembly. --- Signed-off-by: Surya Jasper <45545431+suryajasper@users.noreply.github.com>
comgr's multithreaded tests was commented out because it would break with ASAN builds. The break was a bug in ASAN which has now been [patched upstream](llvm#179000)
accauble
requested review from
a team,
Groverkss,
JDevlieghere,
PeimingLiu,
aaronmondal,
aartbik,
andykaylor,
banach-space,
bcardosolopes,
dcaballe,
fschlimb,
ftynse,
keith,
krzysz00,
kuhar,
lamb-j,
lanza,
matthias-springer,
nicolasvasilache,
rengolin,
rolfmorel,
rupprecht,
xlauko and
yinying-lisa-li
as code owners
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
zizmor found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Contributor
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Re-enable comgr's multithreaded test
A one-line change to re-enable the multithreaded test in comgr which had been commented out because it would break with ASAN builds. The break was a bug in ASAN which has now been patched upstream.
Testing
Build ASAN in LLVM (
-DCOMPILER_RT_BUILD_SANITIZERS=ON). Build comgr with and without ASAN (-DADDRESS_SANITIZER=On). The tests pass in both builds.