Skip to content

[Host IR] Remove alias#4467

Merged
wujingyue merged 5 commits intomainfrom
remove_alias_from_hic
May 22, 2025
Merged

[Host IR] Remove alias#4467
wujingyue merged 5 commits intomainfrom
remove_alias_from_hic

Conversation

@samnordmann
Copy link
Collaborator

@samnordmann samnordmann commented May 16, 2025

aliasing is not needed anymore in Host IR after #4301

@samnordmann
Copy link
Collaborator Author

!test

@github-actions
Copy link

github-actions bot commented May 16, 2025

Review updated until commit 7052bbc

Description

  • Removed alias handling from Host IR container and executor

  • Updated executor methods to directly use expr_evaluator_

  • Removed alias-related tests


Changes walkthrough 📝

Relevant files
Enhancement
container.cpp
Remove alias printing                                                                       

csrc/host_ir/container.cpp

  • Removed alias printing in print method
+0/-7     
executor.cpp
Update executor to remove alias usage                                       

csrc/host_ir/executor.cpp

  • Removed alias checks and usage in multiple methods
  • Updated bind and invalidate calls to use expr_evaluator_ directly
  • +25/-28 
    convert_op_to_communication.cpp
    Remove alias check in pass                                                             

    csrc/host_ir/pass/convert_op_to_communication.cpp

    • Removed alias check in passImplementation
    +1/-2     
    container.h
    Remove alias methods                                                                         

    csrc/host_ir/container.h

    • Removed markAlias, alias, and related methods
    +0/-12   
    executor.h
    Remove alias methods from executor                                             

    csrc/host_ir/executor.h

  • Removed getAlias, isKnown, bind, and invalidate methods
  • Updated getKnownConcreteValue and getKnownTensorOrUndefined to use
    expr_evaluator_ directly
  • +4/-22   
    Tests
    test_host_irs.cpp
    Remove alias tests                                                                             

    tests/cpp/test_host_irs.cpp

    • Removed alias-related tests
    +0/-78   

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🧪 No relevant tests
    ⚡ Recommended focus areas for review

    Possible Issue

    The expr_evaluator_.bind method is now directly used instead of bind which previously handled alias resolution. Ensure that this change does not introduce any issues related to alias handling.

    expr_evaluator_.bind(in_val, arg);
    Possible Issue

    The expr_evaluator_.isKnown method is now directly used instead of isKnown which previously handled alias resolution. Ensure that this change does not introduce any issues related to alias handling.

    void HostIrEvaluator::handle(LinearOp* linear) {
    Missing Tests

    The tests related to alias handling (HirAlias) have been removed. Ensure that the functionality previously covered by these tests is still adequately tested or that new tests are added to cover the same scenarios.

    @samnordmann samnordmann requested review from nsarka and wujingyue May 16, 2025 16:43
    Copy link
    Collaborator

    @wujingyue wujingyue left a comment

    Choose a reason for hiding this comment

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

    Thank you!

    Copy link
    Member

    @nsarka nsarka left a comment

    Choose a reason for hiding this comment

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

    Looks good! For my own understanding, in the HirAliasSelect IR, why not also make the dim a Val*?

    @samnordmann
    Copy link
    Collaborator Author

    !test

    @samnordmann
    Copy link
    Collaborator Author

    samnordmann commented May 19, 2025

    Looks good! For my own understanding, in the HirAliasSelect IR, why not also make the dim a Val*?

    There is no technical restriction for making it a Val*, but I did not for the following reason:

    • Mimick what is done for SelectOp
    • This follows the general idea that axis's extent are dynamic (i.e. Val), but the "number of axis" and each "axis index" themselves are static. Since dim is an axis's index, it makes sense to keep it a int64_t

    Does it make sense ?

    @samnordmann samnordmann force-pushed the remove_alias_from_hic branch from 4194737 to 0c85cba Compare May 19, 2025 15:42
    @samnordmann
    Copy link
    Collaborator Author

    !test

    1 similar comment
    @samnordmann
    Copy link
    Collaborator Author

    !test

    @samnordmann
    Copy link
    Collaborator Author

    !test

    @samnordmann
    Copy link
    Collaborator Author

    !test

    @samnordmann
    Copy link
    Collaborator Author

    !test

    @wujingyue wujingyue merged commit 697c696 into main May 22, 2025
    47 of 50 checks passed
    @wujingyue wujingyue deleted the remove_alias_from_hic branch May 22, 2025 23:08
    jacobhinkle pushed a commit that referenced this pull request May 23, 2025
    aliasing is not needed anymore in Host IR after
    #4301
    nsarka pushed a commit to nsarka/Fuser that referenced this pull request Jul 28, 2025
    aliasing is not needed anymore in Host IR after
    NVIDIA#4301
    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