Repository navigation
[rocjitsu] Make ISA targets statically pluggable - #9099
Conversation
❌ PR Check — Action Required
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
amd-arosa
left a comment
There was a problem hiding this comment.
I sent you two patches to align with the style guide.
There might be something funny happening with create_decoder_for_target, but it was pre-existing.
but other than that LGTM.
388fb06 to
14e6fe5
Compare
amd-eochoalo
left a comment
There was a problem hiding this comment.
I think this is good. Not sure if it would be interesting / good to link kTargetDescriptor to isa_properties somehow in the future (but probably not worth while on this PR at the moment).
There was a problem hiding this comment.
Approved with a minors from the agent:
MINOR
[MINOR] emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/
gfx1250/CMakeLists.txt:71 — Full gfx1250 provider drops its object
libraries’ link requirements
Reasoning: rocjitsu_isa_gfx1250 exposes the model and execution objects
through $<TARGET_OBJECTS:...>, but it does not link or otherwise propagate
the underlying object-library targets. Those targets receive util and
simdojo_headers requirements through rj_add_object_library().
This differs from normal object-library providers: the registry helper both
includes their objects and links the provider target so its usage
requirements propagate. Because the full gfx1250 provider is an
INTERFACE_LIBRARY, the registry helper cannot discover the underlying model
and execution targets. Current broad in-tree consumers independently link
the missing dependencies, masking the composition gap. A narrow or
downstream consumer can therefore depend on undocumented transitive setup
or fail to link as these object libraries acquire additional non-header-
only dependencies.
Evidence:
-
gfx1250/CMakeLists.txt:35-64 creates the model and execution object
libraries through rj_add_object_library(). -
gfx1250/CMakeLists.txt:71-77 forwards only their object files.
-
cmake/rj_add_object_library.cmake:27-30 attaches common dependencies to
each object library. -
cmake/rj_add_object_library.cmake:101-104 extracts objects only when the
registered provider is itself an object library. -
cmake/rj_add_object_library.cmake:134-139 republishes the full interface
provider, whose interface currently contains no link dependencies. -
docs/isa-target-providers.md:66-87 presents the provider/registry
combination as the consumer-facing composition boundary.
Suggested fix: retain the explicit $<TARGET_OBJECTS:...> entries, but
propagate both underlying targets’ usage requirements:
target_link_libraries(
rocjitsu_isa_gfx1250
INTERFACE
rocjitsu_isa_gfx1250_model
rocjitsu_isa_gfx1250_exec
)
Alternatively, extend the provider helper to record and forward
implementation dependencies. Add a narrow final-image link test that
consumes the full gfx1250 registry without also inheriting the simulator’s
broad dependency list.
Confidence: medium-high. The usage-requirement propagation gap is definite;
existing production targets currently mask it, so I did not reproduce an
in-tree link failure.
14e6fe5 to
4c6d2f4
Compare
Make ISA targets statically pluggable and simplify integration of downstream NPI targets without adding them to central target switches. - Add a constexpr descriptor to each ISA that owns its names, public and ELF bindings, decoder factory, and execution capability. - Compose one validated immutable registry per final image from its selected provider headers, and use it for decoder creation and code-object lookup. - Add canonical-name decoder creation with recoverable unavailable-target errors. - Remove fine-grained gfx1250 execution registration markers and replace them with one generated immutable callback table while preserving model-only decoding. N/A Run ctest and pytest. Passed. - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.
4c6d2f4 to
5e30a63
Compare
## Motivation Make ISA targets statically pluggable and simplify integration of downstream NPI targets without adding them to central target switches. ## Technical Details - Add a constexpr descriptor to each ISA that owns its names, public and ELF bindings, decoder factory, and execution capability. - Compose one validated immutable registry per final image from its selected provider headers, and use it for decoder creation and code-object lookup. - Add canonical-name decoder creation with recoverable unavailable-target errors. - Remove fine-grained gfx1250 execution registration markers and replace them with one generated immutable callback table while preserving model-only decoding. ## Issue Tracking N/A ## Test Plan Run ctest and pytest. ## Test Result Passed. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.
PR #7470 had a bad merge with PRs #8933 (intended to bring in register_exec_fn) and #9099 (intended to remove register_exec_fn) in _generator.py. Prior to PR #7470, develop didn't have this function and PR #7470 brought it back in when it shouldn't. Regeneration currently fails on develop, this fixes it. No changes to generated ISA. ## Test Plan Reran all rocjitsu tests ## Test Result All pass ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.
Motivation
Make ISA targets statically pluggable and simplify integration of downstream NPI targets without adding them to central target switches.
Technical Details
Issue Tracking
N/A
Test Plan
Run ctest and pytest.
Test Result
Passed.
Submission Checklist