Improve error message for PrecomputedValues

[zasdfgbnm]

No description provided.

[zasdfgbnm]

!test

[zasdfgbnm]

!test

[github-actions]

Review updated until commit 1b38d3b

Description

• Enhanced error messages in PrecomputedValues validation by including IR node information

• Added IR node tracking to bindValue calls throughout the codebase

• Updated binding_log_ data structure to store IR nodes alongside indices and values

• Modified validate() function to provide detailed error messages with computed vs expected values

Changes walkthrough

Relevant files

Enhancement

evaluator_common.cpp Enhanced error messages with IR node context                          csrc/evaluator_common.cpp Updated all bindValue calls to include IR node parameter (extent/input/ir_node) Completely rewrote validate() function with detailed error messages Enhanced error output to include IR node string representation when available Added computed vs expected value comparison in validation errors +26/-16  evaluator_common.h Updated method signatures for IR node tracking                      csrc/evaluator_common.h Modified bindValue_ method signature to accept optional ir_node parameter Updated bindValue template method to pass through ir_node parameter Changed binding_log_ from vector of pairs to vector of tuples to store IR nodes Added documentation for ir_node parameter usage in error message improvement +10/-5

PR Reviewer Guide

Here are some key observations to aid the review process:

🧪 No relevant tests

⚡ Recommended focus areas for review

Error Message Validation The enhanced error messages in the validate() method look comprehensive, but the PR doesn't include tests to verify that the new error messages are actually displayed correctly when validation fails. Consider adding tests that intentionally cause validation failures to ensure the improved error messages work as expected. void PrecomputedValues::validate() { FUSER_PERF_SCOPE("PrecomputedValuess::Validate"); using namespace PolymorphicValue_functions; for (const auto& [index, expected_value, ir_node] : binding_log_) { if (!isSame(values_[index], expected_value)) { std::stringstream error_msg; error_msg << "Precomputed values failed to validate.\n" << "Something unexpected changed between the compilation and " "execution.\n"; if (ir_node != nullptr) { error_msg << "IR node: " << ir_node->toString() << "\n"; } error_msg << "Computed value: " << values_[index] << "\n" << "Expected value: " << expected_value; NVF_ERROR(false, error_msg.str()); } } has_valid_values_ = true; } API Design Consistency The changes maintain backward compatibility by making the ir_node parameter optional, but there are now two bindValue methods (bindValue_ and bindValue template). Ensure this doesn't cause any ambiguity or unexpected overload resolution issues in existing code. void bindValue_( int index, const PolymorphicValue& value, const Val* ir_node = nullptr) { if (index < 0 || is_constant_[index]) { return; } defined_[index] = true; values_[index] = value; binding_log_.emplace_back(index, value, ir_node); } template <typename T> void bindValue(int index, const T& value, const Val* ir_node = nullptr) { bindValue_(index, PolymorphicValue(value), ir_node); }

[zasdfgbnm]

!test

[zasdfgbnm]

!test

[greptile-apps]

Greptile Summary

This PR enhances error messages in the PrecomputedValues validation system by tracking the original IR nodes associated with bound values. When validation fails, the error message now includes the IR node's string representation, making it easier to identify which value caused the mismatch.

Key changes:

• Modified bindValue_ and bindValue to accept an optional ir_node parameter
• Updated binding_log_ from std::pair<int, PolymorphicValue> to std::tuple<int, PolymorphicValue, const Val*>
• Enhanced validate() to include IR node information in error messages
• Updated all call sites to pass the appropriate IR node when available

Confidence Score: 5/5

• This PR is safe to merge with minimal risk
• The changes are straightforward and focused on improving error messages without altering core logic. All call sites are consistently updated to pass the new parameter, and the implementation uses safe optional parameters with proper null checks
• No files require special attention

Important Files Changed

Filename	Overview
csrc/evaluator_common.h	Updated bindValue_ and bindValue signatures to accept optional ir_node parameter for error tracking, changed binding_log_ from pair to tuple to store IR node information
csrc/evaluator_common.cpp	Updated all bindValue call sites to pass IR node parameter, improved validate() to output IR node information in error messages

Sequence Diagram

sequenceDiagram
    participant Client
    participant PrecomputedValues
    participant bindValue
    participant binding_log
    participant validate

    Client->>PrecomputedValues: bindParallelExtents/bindConcreteParallelTypeValue/bindValues
    PrecomputedValues->>bindValue: bindValue(index, value, ir_node)
    bindValue->>binding_log: emplace_back(index, value, ir_node)
    Note over binding_log: Stores tuple with IR node reference
    
    Client->>PrecomputedValues: evaluate()
    PrecomputedValues->>validate: validate()
    
    alt Values match
        validate->>PrecomputedValues: has_valid_values_ = true
        validate-->>Client: Success
    else Values don't match
        validate->>validate: Build error message with IR node info
        Note over validate: Includes ir_node->toString() if not nullptr
        validate-->>Client: NVF_ERROR with detailed message
    end

Loading

[naoyam]

Could you add a comment about the optional ir_node parameter?

[naoyam]

LGTM

[zasdfgbnm]

!test