[Refactor] Move ConstrVisitor to src/transform/common/constr_visitor.h for reuse#1622
Conversation
|
👋 Hi! Thank you for contributing to the TileLang project. Please remember to run We appreciate you taking this step! Our team will review your contribution, and we look forward to your awesome work! 🚀 |
|
Warning Rate limit exceeded@silentCoder-dev has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 7 minutes and 57 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughExtracts constraint-handling types (Constr, ConstrSet, ConstrVisitor) from Changes
Sequence DiagramsequenceDiagram
participant IR as IR Traversal
participant CV as ConstrVisitor
participant Stack as Constraint Stack
participant CS as ConstrSet
participant Analyzer as arith::Analyzer
IR->>CV: Visit node (If/For/Let/Attr/Assert/Select/Call)
alt Node produces a constraint
CV->>Stack: Push Constr (Guard scope)
note right of Stack `#DDEBF7`: bindings, ranges, assumptions
end
CV->>CV: Traverse children (propagate scoped constraints)
alt Children produce constraints
CV->>Stack: Accumulate constr_stack_
end
alt Guard scope ends
Stack->>Stack: Pop scoped constraints
end
CV->>CS: Aggregate constraints into ConstrSet
CS->>Analyzer: Populate(analyzer)
Analyzer->>CS: Query/CanProve(expr)
CS-->>CV: Proof result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom Pre-merge Checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/transform/common/constr_visitor.h:
- Around line 14-17: The header currently has a top-level "using namespace tir;"
which pollutes all translation units that include it; remove that directive from
the header and fully qualify tir symbols (e.g., tir::Var, tir::PrimExpr,
tir::Range, tir::ForKind) wherever they are used in this header (or,
alternatively, put a local "using tir::Var;"-style alias or a scoped "using
namespace tir;" only inside implementation/function bodies). Update declarations
in constr_visitor.h (types, function signatures, class members) to use explicit
tir:: qualification so the header no longer relies on a global using directive.
- Around line 1-12: The header is missing an include guard which will cause
redefinition on multiple inclusion; add a guard by inserting either `#pragma
once` at the very top of constr_visitor.h or wrap the entire file with an
include guard using a unique macro (e.g., `#ifndef CONSTR_VISITOR_H_` / `#define
CONSTR_VISITOR_H_` ... `#endif`) ensuring the closing `#endif` is placed at the
end of the file.
🧹 Nitpick comments (2)
src/transform/common/constr_visitor.h (1)
179-179: Add trailing newline.The file should end with a newline character after the closing brace.
🔎 Proposed fix
-} +} // namespace tvm::tl +src/transform/verify_parallel_loop.cc (1)
20-23: Redundant using declarations.Since this anonymous namespace is nested within
namespace tvm::tl,ConstrSetandConstrVisitorare already directly accessible without these using declarations.🔎 Proposed fix
namespace { -using tvm::tl::ConstrSet; -using tvm::tl::ConstrVisitor; struct ParallelLoopVerifier : public ConstrVisitor {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/transform/common/constr_visitor.hsrc/transform/verify_parallel_loop.cc
🧰 Additional context used
🧬 Code graph analysis (1)
src/transform/common/constr_visitor.h (2)
src/transform/common/loop_vectorization_utils.h (2)
VisitExpr(196-198)VisitStmt(185-194)src/transform/verify_parallel_loop.cc (6)
op(28-36)op(28-28)op(37-82)op(37-37)op(83-93)op(83-83)
🔇 Additional comments (4)
src/transform/common/constr_visitor.h (3)
18-68: LGTM!The
Constrstruct is well-designed with clear separation of constraint kinds and appropriate methods for conversion, substitution, and population.
70-97: LGTM!The
ConstrSetstruct provides a clean interface for constraint management with appropriate methods for substitution, population, and extension.
99-178: LGTM!The
ConstrVisitorcorrectly implements RAII-based constraint scoping via theGuardhelper, and the visitor overrides properly handle constraint propagation for various node types.src/transform/verify_parallel_loop.cc (1)
1-1: LGTM!The include path correctly references the new common header location.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/transform/common/constr_visitor.h (2)
96-96: Consider makingconstrs_private with accessor methods.The public
constrs_member breaks encapsulation and allows external code to modify the constraint vector directly, bypassing the class interface. For a shared component intended for reuse across the codebase, consider making this private and providing appropriate accessor methods (e.g.,const std::vector<Constr>& GetConstrs() const).🔎 Suggested refactor
+private: + std::vector<Constr> constrs_; + +public: + const std::vector<Constr>& GetConstrs() const { return constrs_; } - std::vector<Constr> constrs_;Then update any external access to use
GetConstrs()instead of direct member access.
177-177: Consider makingconstr_stack_protected instead of public.The public
constr_stack_member exposes internal implementation details. SinceConstrVisitoris designed as a base class for inheritance (as seen withParallelLoopVerifierin the PR context), consider making this memberprotectedto allow subclass access while preventing external modification.🔎 Suggested refactor
+protected: + std::vector<Constr> constr_stack_; +}; - std::vector<Constr> constr_stack_; -};
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/transform/common/constr_visitor.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test for Python 3.12 with Metal (on macos-latest)
🔇 Additional comments (1)
src/transform/common/constr_visitor.h (1)
52-54: No issues identified. TheSubstitutemethod correctly converts constraints to generic form after variable substitution, as the original variables are replaced with substituted ones and can no longer be bindings in the analyzer.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/transform/common/constr_visitor.h:
- Around line 55-67: The switch in Populate (method Populate in
constr_visitor.h) doesn’t handle unexpected Kind values; add a default case that
fails loudly instead of silently returning: in the switch over kind (enum Kind)
add a default branch which logs or throws an error/assertion with the unexpected
kind value and some context (e.g., include the value of kind and the var/value
being processed) so new Kind variants don’t silently bypass
analyzer.EnterConstraint/Bind calls; ensure the default uses the project’s
existing error reporting mechanism (throw, LOG(FATAL)/CHECK, or similar) rather
than a no-op.
- Around line 1-13: The header lacks an include guard; add a header guard to the
top and bottom of this header (either a single-line #pragma once or a pair of
#ifndef/#define ... #endif) so multiple inclusion won’t cause redefinition
errors; if using the traditional guard choose a unique macro name (e.g.
TVM_TRANSFORM_COMMON_CONSTR_VISITOR_H or similar) and place the matching #endif
at the end of the file to cover all existing includes and declarations (e.g.,
the declarations that refer to tvm::tir types and any ConstrVisitor-related
classes/functions).
🧹 Nitpick comments (1)
src/transform/common/constr_visitor.h (1)
179-179: Consider makingconstr_stack_protected.Since
ConstrVisitoris designed as a base class (per the PR summary, it's subclassed byParallelLoopVerifier), theconstr_stack_member should beprotectedrather thanpublicto prevent external access while allowing derived classes to use it.🔎 Proposed fix
+protected: std::vector<Constr> constr_stack_; };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/transform/common/constr_visitor.h
🔇 Additional comments (1)
src/transform/common/constr_visitor.h (1)
70-97: LGTM!The
ConstrSetimplementation provides clean methods for constraint manipulation. The use of variadic templates inAddConstrallows flexible constraint creation.
kurisu6912
left a comment
kurisu6912
left a comment
There was a problem hiding this comment.
AI gives good comments, @silentCoder-dev would be good to go to resolve these comments
3 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.