Skip to content

Revert "Deallocate HostIr Op and Test"#4303

Merged
jjsjann123 merged 1 commit intomainfrom
revert-4286-nsarka/hostir-integration-3
Apr 23, 2025
Merged

Revert "Deallocate HostIr Op and Test"#4303
jjsjann123 merged 1 commit intomainfrom
revert-4286-nsarka/hostir-integration-3

Conversation

@wujingyue
Copy link
Collaborator

@wujingyue wujingyue commented Apr 23, 2025

Reverts #4286 because of http://nv/eF0

@wujingyue wujingyue requested review from jjsjann123 and nsarka April 23, 2025 22:43
@wujingyue
Copy link
Collaborator Author

!test

@github-actions
Copy link

Description

  • Reverts removal of HostIr Deallocate Op and Test

  • Restores Deallocate handling in HostIrEvaluator

  • Removes unused preallocated_outputs check

  • Updates dispatch.h and host_ir.h to include Deallocate


Changes walkthrough 📝

Relevant files
Enhancement
executor.cpp
Restore Deallocate Handling                                                           

csrc/host_ir/executor.cpp

  • Restored Deallocate handling in HostIrEvaluator
  • Removed preallocated_outputs check and related code
  • +12/-36 
    host_ir.cpp
    Remove Deallocate Class                                                                   

    csrc/host_ir/host_ir.cpp

    • Removed Deallocate class definition and related methods
    +0/-23   
    test_host_ir_integration.cpp
    Remove Deallocate Test                                                                     

    tests/cpp/test_host_ir_integration.cpp

    • Removed Deallocate test case
    +0/-26   
    dispatch.h
    Restore Deallocate in Dispatch                                                     

    csrc/dispatch.h

    • Restored Deallocate in dispatch macro
    +1/-2     
    executor.h
    Restore Deallocate Method                                                               

    csrc/host_ir/executor.h

    • Restored Deallocate method declaration
    +0/-1     
    host_ir.h
    Remove Deallocate Class                                                                   

    csrc/host_ir/host_ir.h

    • Removed Deallocate class definition
    +0/-21   

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🧪 No relevant tests
    ⚡ Recommended focus areas for review

    Output Handling

    The new code directly binds outputs without checking if they are preallocated, which could lead to issues if outputs are expected to be preallocated.

    KernelArgumentHolder outputs =
        container_->getKernelExecutor(launch_kernel->getIndex())
            ->run(
                args,
                {},
                launch_kernel->launch_params(),
                launch_kernel->compile_params());
    
    // Store the outputs in the context
    for (auto output_idx : arange(outputs.size())) {
      bind(launch_kernel->outputs().at(output_idx), outputs[output_idx]);
    }
    Removed Functionality

    The Deallocate class and its related methods have been removed, which might affect memory management and could lead to memory leaks if not handled elsewhere.

    std::string LaunchKernel::toInlineString(int indent_size) const {
      NVF_CHECK(false, "Can not be printed inline");
    }
    
    Stream::Stream(IrBuilderPasskey passkey, Val* index)
        : Val(passkey, ValType::Stream), index_(index) {}
    
    Stream::Stream(const Stream* src, IrCloner* ir_cloner)
        : Val(src, ir_cloner), index_(src->index()) {}
    
    NVFUSER_DEFINE_CLONE(Stream)
    
    std::string Stream::toString(int indent_size) const {
      std::stringstream ss;
      indent(ss, indent_size) << "Stream ";
      if (index() == nullptr) {
        ss << name();
      } else {
        ss << index()->toInlineString();
      }
      return ss.str();
    }
    
    std::string Stream::toInlineString(int indent_size) const {
      return toString(indent_size);
    }
    
    bool Stream::sameAs(const Statement* other) const {
      if (other == this) {
        return true;
      }
    Removed Test

    The Deallocate test has been removed, which could lead to a gap in testing memory deallocation functionality.

          {in_tensor},
          __LINE__,
          __FILE__,
          "");
    }
    
    } // namespace hir
    
    } // namespace nvfuser

    const std::vector<int64_t> sizes = {8, 64};
    c10::DeviceIndex device_index = 0;

    resetPeakMemoryStats(device_index);
    Copy link
    Collaborator Author

    Choose a reason for hiding this comment

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

    @nsarka I suspect you need something like

    at::cuda::clearCublasWorkspaces();
    releaseZeroedMemory();
    ASSERT_EQ(memoryAllocated(0), 0) << "Previous tests leaked memory.";

    Copy link
    Collaborator

    @jjsjann123 jjsjann123 left a comment

    Choose a reason for hiding this comment

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

    🙇

    @jjsjann123 jjsjann123 merged commit 85f9894 into main Apr 23, 2025
    24 of 27 checks passed
    @jjsjann123 jjsjann123 deleted the revert-4286-nsarka/hostir-integration-3 branch April 23, 2025 23:41
    nsarka added a commit to nsarka/Fuser that referenced this pull request Apr 29, 2025
    nsarka added a commit to nsarka/Fuser that referenced this pull request Apr 29, 2025
    nsarka added a commit that referenced this pull request Apr 29, 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.

    2 participants