Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove conditional compilation #51

Closed
broskoTT opened this issue Sep 10, 2024 · 11 comments · Fixed by #87, #88 or #89
Closed

Remove conditional compilation #51

broskoTT opened this issue Sep 10, 2024 · 11 comments · Fixed by #87, #88 or #89
Assignees
Labels
enhancement New feature or request P1 Major issues with workarounds available, fix soon repo health Code design, code readability, linting, file restructuring, etc. tt-metal pain point tt-metal team has interest in this change

Comments

@broskoTT
Copy link
Contributor

Examine uses of conditional compilation (e.g. ARCH_NAME, ISSUE_123_DISABLE_FIX, etc.) and eliminate where possible.

@broskoTT broskoTT added enhancement New feature or request repo health Code design, code readability, linting, file restructuring, etc. labels Sep 10, 2024
@vtangTT vtangTT mentioned this issue Sep 17, 2024
4 tasks
@broskoTT broskoTT added the tt-metal pain point tt-metal team has interest in this change label Sep 21, 2024
@broskoTT
Copy link
Contributor Author

Conditional compilation for ARCH_NAME was removed by @tt-vjovanovic a while back. But it still remains in CMakeLists.
Make sure to remove it as part of this task.

@joelsmithTT
Copy link
Contributor

The DISABLE_ISSUE_3487_FIX define can be made a runtime thing. Here's additional context for that issue:
https://yyz-gitlab.local.tenstorrent.com/tenstorrent/syseng/-/issues/3487
https://yyz-gitlab.local.tenstorrent.com/tenstorrent/syseng/-/merge_requests/1149/diffs

broskoTT added a commit that referenced this issue Sep 30, 2024
A continuation to #87 , to reflect the current state in our CI.
Related to #86 and a part of #51 .

There is now enough difference between building umd_device and umd_tests
to warrant separate files, since umd_tests still requires ARCH_NAME and
umd_device does not. This way it looks clearer.
@broskoTT
Copy link
Contributor Author

broskoTT commented Oct 9, 2024

Is this issue now complete? I do see some isolated cases of ifdefs, but looks like the two top offenders are now resolved? What do you think @joelsmithTT @pjanevskiTT

@pjanevskiTT
Copy link
Contributor

pjanevskiTT commented Oct 9, 2024

As for as umd_device target, I think it is complete. However, we still have tests. I am working on removing soc descriptor from tt_SiliconDevice constructor, after that we can have unified compilation for tests as well. If this was never about the tests, we can close it

@joelsmithTT
Copy link
Contributor

The code part is more or less complete (no more ARCH_NAME in source files) but as @pjanevskiTT points out there is still a bit more work to do for the tests. (ARCH_NAME still shows up in tests/CMakeLists.txt and controls include paths).

@blozano-tt
Copy link
Collaborator

Speaking of tests:

  • I think we still need to get rid of ARCH_NAME ENV

I think we have a few options, but the easiest that is coming to mind is some CMake magic to generate 3x the number of executables.

i.e. a custom add_executable

umd_add_exectuable(target_name arch_name)

Use the above in a for each loop.

cc: @afuller-TT

@blozano-tt
Copy link
Collaborator

function(umd_add_executable target arch interface_lib)
    # Create the executable target
    add_executable(${target})

    # Link the interface library to the new target
    target_link_libraries(${target} PRIVATE ${interface_lib})

    # Set the compile definitions based on the architecture
    if(${arch} STREQUAL "ARCH1")
        target_compile_definitions(${target} PRIVATE DEFINE_ARCH1)
        target_include_directories(${target} PRIVATE ${CMAKE_SOURCE_DIR}/path/to/include_arch1)
    elseif(${arch} STREQUAL "ARCH2")
        target_compile_definitions(${target} PRIVATE DEFINE_ARCH2)
        target_include_directories(${target} PRIVATE ${CMAKE_SOURCE_DIR}/path/to/include_arch2)
    elseif(${arch} STREQUAL "ARCH3")
        target_compile_definitions(${target} PRIVATE DEFINE_ARCH3)
        target_include_directories(${target} PRIVATE ${CMAKE_SOURCE_DIR}/path/to/include_arch3)
    else()
        message(FATAL_ERROR "Unknown architecture: ${arch}")
    endif()
endfunction()

@afuller-TT
Copy link
Collaborator

@blozano-tt

I think we have a few options, but the easiest that is coming to mind is some CMake magic to generate 3x the number of executables.

Executable? or lib?
If the arch logic is all pushed down to a lib that we load a runtime, then everything everywhere could be a single executable, right?

If this is actual unit tests (no hw involved) then doing a matrix of tests in a single executable would suffice, no?

@blozano-tt
Copy link
Collaborator

Executable? or lib? If the arch logic is all pushed down to a lib that we load a runtime, then everything everywhere could be a single executable, right?

I was talking about all the test exes, unfortunately the test cases need to be ARCH_NAME aware.

If this is actual unit tests (no hw involved) then doing a matrix of tests in a single executable would suffice, no?

I am not familiar with all the tests in the repo, but there are at least some that require hardware.

@pjanevskiTT
Copy link
Contributor

@blozano-tt @afuller-TT can you open smaller issues for separate tests where the aim would be to remove conditional compilation and close this bigger issue?

@pjanevskiTT pjanevskiTT added the P1 Major issues with workarounds available, fix soon label Nov 5, 2024
@broskoTT
Copy link
Contributor Author

broskoTT commented Nov 6, 2024

Closed in favor of #270 capturing further progress

@broskoTT broskoTT closed this as completed Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request P1 Major issues with workarounds available, fix soon repo health Code design, code readability, linting, file restructuring, etc. tt-metal pain point tt-metal team has interest in this change
Projects
None yet
5 participants