Skip to content

Conversation

@wujingyue
Copy link
Collaborator

NFC. Needed by #5229.

This way, I can pass round only the map without the containing fusion.

NFC. Needed by #5229.

This way, I can pass round only the map without the containing fusion.
@wujingyue
Copy link
Collaborator Author

!test

@github-actions
Copy link

github-actions bot commented Sep 25, 2025

Review updated until commit a081662

Description

  • Replace raw map with AliasInfoMap for better alias management

  • Introduce OutputVisibility enum to replace boolean hide_output

  • Update alias handling logic across multiple components

  • Add helper methods for safer access to alias information


Changes walkthrough 📝

Relevant files
Enhancement
fusion.cpp
Implement AliasInfoMap and update fusion logic                     

csrc/fusion.cpp

  • Replace direct map access with AliasInfoMap methods
  • Use get() and mutable_at() for safer alias access
  • Convert hide_output boolean to OutputVisibility enum
  • Add operator<< for OutputVisibility and AliasInfoMap implementation
  • +61/-26 
    fusion.h
    Define OutputVisibility and AliasInfoMap                                 

    csrc/fusion.h

  • Add OutputVisibility enum
  • Define AliasInfoMap class with proper interface
  • Update AliasInfo to use OutputVisibility instead of bool
  • Change io_alias_ type to AliasInfoMap
  • +34/-5   
    Bug fix
    fusion_executor_cache.cpp
    Update output visibility check                                                     

    csrc/runtime/fusion_executor_cache.cpp

  • Update hide_output check to use OutputVisibility
  • Replace boolean comparison with enum comparison
  • +1/-1     
    validator_utils.cpp
    Update validation output filtering                                             

    csrc/validator_utils.cpp

  • Replace hide_output check with OutputVisibility comparison
  • Update lambda to use visibility enum
  • +2/-1     
    distributed_tensor.cpp
    Update sharding output filtering                                                 

    python/python_common/distributed_tensor.cpp

  • Replace hide_output check with OutputVisibility comparison
  • Update conditional to use enum value
  • +2/-1     
    python_translate.cpp
    Update Python translation output handling                               

    python/python_direct/python_translate.cpp

  • Replace hide_output check with OutputVisibility comparison
  • Update conditional logic for output handling
  • +1/-1     
    translation.cpp
    Update frontend translation output handling                           

    python/python_frontend/translation.cpp

  • Replace hide_output check with OutputVisibility comparison
  • Update comment to reflect enum usage
  • +3/-2     

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🧪 PR contains tests
    ⚡ Recommended focus areas for review

    Possible Issue

    The new add method in AliasInfoMap uses try_emplace and expects insertion to succeed, but the error message uses aliases_.at(out) which may throw if the key is not present, creating a contradiction in error handling.

      auto [_, inserted] = aliases_.try_emplace(
          out, AliasInfo{.type = type, .aliased_io = in, .visibility = visibility});
      NVF_ERROR(
          inserted,
          "The map already has an AliasInfo for ",
          out,
          ": ",
          aliases_.at(out).toString());
    }
    Logic Consistency

    The get method in AliasInfoMap returns a static no_alias_info for missing entries, but mutable_at directly uses at which throws if the key is not present. This inconsistency could lead to undefined behavior if mutable_at is called on a non-existent key.

    const AliasInfo& AliasInfoMap::get(const Val* v) const {
      static AliasInfo no_alias_info{
          .type = AllocationType::New,
          .aliased_io = nullptr,
          .visibility = OutputVisibility::kVisible};
      if (auto search = aliases_.find(v); search != aliases_.end()) {
        return search->second;
      }
      return no_alias_info;
    }

    @wujingyue
    Copy link
    Collaborator Author

    !test

    @wujingyue wujingyue marked this pull request as ready for review September 26, 2025 01:59
    Copy link
    Collaborator

    @Priya2698 Priya2698 left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    This way, I can pass round only the map without the containing fusion.

    Even with the existing design, we can pass a map to lowerSegment. The change seems to be that instead of an unordered map, we pass AliasInfoMap. The changes look fine to me, but I don't understand the above motivation completely. I maybe missing something here.

    @wujingyue
    Copy link
    Collaborator Author

    Even with the existing design, we can pass a map to lowerSegment.

    That's right. Currently, I'm just trying to hide implementation details like https://github.com/NVIDIA/Fuser/pull/5239/files#diff-2aba2455a2f4f8064111d33fb49a7416ffab0f013153f868e5fbaa5309dd5384R144-R151 behind the AliasInfoMap API and other methods of std::unordered_map

    @wujingyue wujingyue requested a review from Priya2698 September 26, 2025 18:50
    @wujingyue
    Copy link
    Collaborator Author

    !test

    @wujingyue wujingyue merged commit bea74f1 into main Sep 26, 2025
    25 of 26 checks passed
    @wujingyue wujingyue deleted the wjy/alias branch September 26, 2025 23:26
    wujingyue added a commit that referenced this pull request Sep 27, 2025
    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.

    3 participants