[Refactor] Disable Predicated LDG PTX Lowering by default #1739
Conversation
- Changed the configuration option from `kDisableLowerLDGSTGPredicated` to `kEnableLowerLDGSTGPredicated` to enable predicated lowering of global load/store operations. - Updated relevant comments and documentation to reflect the new configuration behavior. - Adjusted tests to utilize the new configuration option for enabling predicated lowering, ensuring consistency across the codebase.
|
👋 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! 🚀 |
📝 WalkthroughWalkthroughRefactoring of a public pass configuration flag across the codebase: the boolean option controlling LDG/STG predicated lowering is renamed from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
🤖 Fix all issues with AI agents
In `@testing/python/transform/test_tilelang_transform_lower_ldgstg.py`:
- Line 138: Update the misleading inline comments that state "Default:
predicated is ON" next to calls to _apply_passes(..., enable_predicated=True);
the new semantics default predication to OFF, so change those comments (the ones
adjacent to the _apply_passes calls referencing enable_predicated=True) to
reflect "Default: predicated is OFF" or remove the misleading default note so
the comment matches the actual behavior.
🧹 Nitpick comments (1)
src/transform/lower_ldg_stg.cc (1)
301-304: Consider updating the default member initializer for consistency.The member
enable_predicated_is initialized totrueat line 304, but the constructor (lines 42-45) always overrides this value with the parameter passed from the pass function. Since the pass now defaults tofalse(line 496), updating this member initializer tofalsewould improve code clarity and maintain consistency with the new default semantics.♻️ Suggested change
private: bool in_async_scope_{false}; bool enable_non_predicated_{false}; - bool enable_predicated_{true}; + bool enable_predicated_{false};
|
|
||
| mod = tvm.IRModule.from_expr(func.with_attr("global_symbol", "main")) | ||
| mod = _apply_passes(mod) # Default: predicated is ON | ||
| mod = _apply_passes(mod, enable_predicated=True) # Default: predicated is ON |
There was a problem hiding this comment.
Misleading inline comment.
The comment says "Default: predicated is ON" but with the new semantics, the default is OFF. The test correctly passes enable_predicated=True to enable it, but the comment should be updated.
📝 Suggested fix
- mod = _apply_passes(mod, enable_predicated=True) # Default: predicated is ON
+ mod = _apply_passes(mod, enable_predicated=True) # Explicitly enable predicated loweringThe same applies to lines 155, 172, and 190.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mod = _apply_passes(mod, enable_predicated=True) # Default: predicated is ON | |
| mod = _apply_passes(mod, enable_predicated=True) # Explicitly enable predicated lowering |
🤖 Prompt for AI Agents
In `@testing/python/transform/test_tilelang_transform_lower_ldgstg.py` at line
138, Update the misleading inline comments that state "Default: predicated is
ON" next to calls to _apply_passes(..., enable_predicated=True); the new
semantics default predication to OFF, so change those comments (the ones
adjacent to the _apply_passes calls referencing enable_predicated=True) to
reflect "Default: predicated is OFF" or remove the misleading default note so
the comment matches the actual behavior.
1 participant
As our benchmark shows that the performance is not always better than the default nvcc optimiztaions.
kDisableLowerLDGSTGPredicatedtokEnableLowerLDGSTGPredicatedto enable predicated lowering of global load/store operations.Summary by CodeRabbit
Truerather than disabling it whenTrue. Users with existing configurations using the old flag will need to update their settings accordingly.✏️ Tip: You can customize this high-level summary in your review settings.