Skip to content

[Bugfix] Fix index handling to promote 64-bit integers #796

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.

Sign up for GitHub

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

Merged
LeiWang1999 merged 1 commit into from
Sep 9, 2025
Merged

[Bugfix] Fix index handling to promote 64-bit integers #796

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/transform/config_index_bitwidth.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,35 +123,43 @@ class IndexLegalizer : public IRMutatorWithAnalyzer {
auto buffer_store =
Downcast<BufferStore>(IRMutatorWithAnalyzer::VisitStmt_(op));
auto indices = buffer_store->indices;
Array<PrimExpr> new_indices;
for (auto index : indices) {
if (index->dtype.is_int() && index->dtype.bits() < 64) {
auto int_bound = analyzer_->const_int_bound(index);
if (int_bound->max_value >= (1LL << (index->dtype.bits() - 1)) - 1 ||
int_bound->min_value < -(1LL << (index->dtype.bits() - 1))) {
Comment on lines 130 to 131
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 9, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Verify overflow detection logic for signed integer bounds.

The overflow detection uses (1LL << (index->dtype.bits() - 1)) - 1 for max_value and -(1LL << (index->dtype.bits() - 1)) for min_value. This appears to check if the value exceeds the representable range for the current bit width, but the logic seems to promote indices that are already at the boundary of their type's range.

Let me verify the signed integer overflow detection logic:

Also applies to: 152-153

🏁 Script executed:

#!/bin/bash
# Description: Verify the overflow detection logic for different integer bit widths
# Expected: Check if the boundary calculations match standard signed integer ranges

python3 << 'EOF'
def check_signed_bounds(bits):
    # Current logic from the code
    max_boundary = (1 << (bits - 1)) - 1
    min_boundary = -(1 << (bits - 1))
    
    # Standard signed integer bounds
    actual_max = (1 << (bits - 1)) - 1
    actual_min = -(1 << (bits - 1))
    
    print(f"{bits}-bit signed integer:")
    print(f"  Code logic: max >= {max_boundary} or min < {min_boundary}")
    print(f"  Actual range: [{actual_min}, {actual_max}]")
    print(f"  Logic promotes when: value >= {max_boundary} or value < {min_boundary}")
    print(f"  This means promotion occurs for values AT the boundary")
    print()

# Test common bit widths
for bits in [8, 16, 32]:
    check_signed_bounds(bits)
EOF

Length of output: 652

Fix signed overflow check to exclude boundary value
In src/transform/config_index_bitwidth.cc (lines 130–131), the current condition

if (int_bound->max_value >= (1LL << (index->dtype.bits() - 1)) - 1 ||
    int_bound->min_value <  -(1LL << (index->dtype.bits() - 1))) {

will promote indices already equal to the max representable value (e.g. 127 for 8 bits). Change the max‐side comparison to > so only values exceeding the range trigger promotion:

- if (int_bound->max_value >= (1LL << (index->dtype.bits() - 1)) - 1 ||
+ if (int_bound->max_value >  (1LL << (index->dtype.bits() - 1)) - 1 ||
     int_bound->min_value <  -(1LL << (index->dtype.bits() - 1))) {
🤖 Prompt for AI Agents 
In src/transform/config_index_bitwidth.cc around lines 130-131, the signed
overflow check currently uses a >= comparison on the max side which will
incorrectly treat values equal to the type's maximum (e.g., 127 for 8-bit) as
overflow; change the max-side comparison from >= to > so only values strictly
greater than the representable maximum trigger promotion, leaving the min-side
check as-is.

Int64Promoter promoter;
index = promoter(index);
new_indices.push_back(index);
continue;
}
}
new_indices.push_back(index);
}
Comment on lines 127 to 139
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Sep 9, 2025

Choose a reason for hiding this comment

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

medium

The logic inside this for-loop can be simplified by removing the continue and the redundant push_back call. This improves readability.

    for (auto index : indices) {
      if (index->dtype.is_int() && index->dtype.bits() < 64) {
        auto int_bound = analyzer_->const_int_bound(index);
        if (int_bound->max_value >= (1LL << (index->dtype.bits() - 1)) - 1 ||
            int_bound->min_value < -(1LL << (index->dtype.bits() - 1))) {
          Int64Promoter promoter;
          index = promoter(index);
        }
      }

      new_indices.push_back(index);
    }

buffer_store.CopyOnWrite()->indices = indices;
buffer_store.CopyOnWrite()->indices = new_indices;
return std::move(buffer_store);
}

PrimExpr VisitExpr_(const BufferLoadNode *op) final {
auto buffer_load =
Downcast<BufferLoad>(IRMutatorWithAnalyzer::VisitExpr_(op));
auto indices = buffer_load->indices;
Array<PrimExpr> new_indices;
for (auto index : indices) {
if (index->dtype.is_int() && index->dtype.bits() < 64) {
auto int_bound = analyzer_->const_int_bound(index);
if (int_bound->max_value >= (1LL << (index->dtype.bits() - 1)) - 1 ||
int_bound->min_value < -(1LL << (index->dtype.bits() - 1))) {
Int64Promoter promoter;
index = promoter(index);
new_indices.push_back(index);
continue;
}
}
new_indices.push_back(index);
}
Comment on lines 149 to 161
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Sep 9, 2025

Choose a reason for hiding this comment

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

medium

This loop can be simplified in the same way as in VisitStmt_. Additionally, the logic in VisitStmt_(const BufferStoreNode *op) and VisitExpr_(const BufferLoadNode *op) is identical after this change. It would be good to extract this logic into a private helper function to avoid code duplication and improve maintainability.

    for (auto index : indices) {
      if (index->dtype.is_int() && index->dtype.bits() < 64) {
        auto int_bound = analyzer_->const_int_bound(index);
        if (int_bound->max_value >= (1LL << (index->dtype.bits() - 1)) - 1 ||
            int_bound->min_value < -(1LL << (index->dtype.bits() - 1))) {
          Int64Promoter promoter;
          index = promoter(index);
        }
      }

      new_indices.push_back(index);
    }

buffer_load.CopyOnWrite()->indices = indices;
buffer_load.CopyOnWrite()->indices = new_indices;
return std::move(buffer_load);
}
};
Expand Down
Loading