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

a/b/c => a/(b*c) if b and c are constant #3954

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

a/b/c => a/(b*c) if b and c are constant #3954

wants to merge 1 commit into from

Conversation

wujingyue
Copy link
Collaborator

No description provided.

@wujingyue
Copy link
Collaborator Author

!test --diff

Copy link

Description

  • Enhance division simplification for constant divisors

  • Add logic to simplify a/b/c to a/(b*c) if b and c are constants


Changes walkthrough 📝

Relevant files
Enhancement
expr_simplifier.cpp
Add division simplification for constant divisors               

csrc/expr_simplifier.cpp

  • Add logic to handle division simplification when the dividend is also
    a division
  • Check if both divisor and sub-divisor are constants
  • Replace a/b/c with a/(b*c) if b and c are constants
  • +14/-0   

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🧪 No relevant tests
    ⚡ Recommended focus areas for review

    Possible Issue

    The new code assumes that lhs_def->getBinaryOpType() is always BinaryOpType::Div when op == BinaryOpType::Div. This might not be the case, and the code should handle other binary operations or add a check to ensure this assumption is valid.

    if (op == BinaryOpType::Div) {
      if (BinaryOp* lhs_def = toDivModOp(lhs->definition())) {
        if (lhs_def->getBinaryOpType() == BinaryOpType::Div) {
          Val* lhs_lhs = lhs_def->lhs();
          Val* lhs_rhs = lhs_def->rhs();
          if (lhs_rhs->isConst() && rhs->isConst()) {
            return IrBuilder::divExpr(lhs_lhs, IrBuilder::mulExpr(lhs_rhs, rhs));
          }
        }
      }
    }
    Code Clarity

    The nested if statements and the logic for checking constant values can be refactored for better readability and maintainability.

    if (op == BinaryOpType::Div) {
      if (BinaryOp* lhs_def = toDivModOp(lhs->definition())) {
        if (lhs_def->getBinaryOpType() == BinaryOpType::Div) {
          Val* lhs_lhs = lhs_def->lhs();
          Val* lhs_rhs = lhs_def->rhs();
          if (lhs_rhs->isConst() && rhs->isConst()) {
            return IrBuilder::divExpr(lhs_lhs, IrBuilder::mulExpr(lhs_rhs, rhs));
          }
        }
      }
    }
    Performance Impact

    The new transformation might have performance implications. It is important to evaluate the performance impact of this change and ensure that it provides a significant benefit.

    if (op == BinaryOpType::Div) {
      if (BinaryOp* lhs_def = toDivModOp(lhs->definition())) {
        if (lhs_def->getBinaryOpType() == BinaryOpType::Div) {
          Val* lhs_lhs = lhs_def->lhs();
          Val* lhs_rhs = lhs_def->rhs();
          if (lhs_rhs->isConst() && rhs->isConst()) {
            return IrBuilder::divExpr(lhs_lhs, IrBuilder::mulExpr(lhs_rhs, rhs));
          }
        }
      }
    }

    Val* lhs_lhs = lhs_def->lhs();
    Val* lhs_rhs = lhs_def->rhs();
    if (lhs_rhs->isConst() && rhs->isConst()) {
    return IrBuilder::divExpr(lhs_lhs, IrBuilder::mulExpr(lhs_rhs, rhs));
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    nit:

    Suggested change
    return IrBuilder::divExpr(lhs_lhs, IrBuilder::mulExpr(lhs_rhs, rhs));
    return IrBuilder::divExpr(lhs_lhs, SimplifyingIrBuilder::mulExpr(lhs_rhs, rhs));

    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