[Layout] Utilizing IsEqual instead of StructuralEqual #1073
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! 🚀 |
WalkthroughA runtime layout equality check in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/transform/layout_inference.cc(1 hunks)
⏰ 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). (2)
- GitHub Check: Test for Python 3.12 with ROCm-6.3 (on self-hosted-amd)
- GitHub Check: Test for Python 3.12 with Metal (on macos-latest)
| ICHECK(layout->IsEqual(layout_map[buffer].get())) | ||
| << "Get different layout for " << buffer | ||
| << "\n current layout: " << layout->DebugOutput() | ||
| << "\n previous layout: " << layout_map[buffer]->DebugOutput(); |
There was a problem hiding this comment.
🧩 Analysis chain
Good improvement using specialized equality.
Replacing StructuralEqual() with layout->IsEqual() is appropriate here, as it leverages domain-specific equality logic for Layout objects rather than generic structural comparison.
To ensure consistency across the codebase, verify that all layout-to-layout comparisons use IsEqual() rather than StructuralEqual():
🏁 Script executed:
#!/bin/bash
# Description: Find potential layout equality comparisons that might need updating.
# Search for StructuralEqual usage with layout-related patterns.
echo "=== Searching for StructuralEqual() calls that might involve Layout objects ==="
rg -n -C3 'StructuralEqual\(\).*layout' --type=cpp
echo -e "\n=== Searching for layout variables being compared ==="
rg -n -C3 'layout.*StructuralEqual|StructuralEqual.*layout' --type=cppLength of output: 6136
🏁 Script executed:
#!/bin/bash
# Find Layout class definition and IsEqual method
echo "=== Searching for Layout class and IsEqual method ==="
rg -n -A5 'class Layout' --type=cpp | head -40
echo -e "\n=== Searching for IsEqual method in Layout ==="
rg -n -B2 -A8 'IsEqual\s*\(' --type=cpp --type=h | grep -A10 -B2 Layout | head -50
echo -e "\n=== Checking types in src/op/copy.cc around line 1468 ==="
sed -n '1450,1485p' src/op/copy.cc
echo -e "\n=== Checking if shared_layout is a Layout type ==="
rg -n -B10 'shared_layout' src/op/copy.cc | grep -E 'shared_layout|Layout|auto' | head -20Length of output: 8292
🏁 Script executed:
#!/bin/bash
# Find Layout class definition and IsEqual method
echo "=== Searching for Layout class and IsEqual method ==="
rg -n -A5 'class Layout' --type=cpp --type=h | head -60
echo -e "\n=== Checking types in src/op/copy.cc around line 1468 ==="
sed -n '1450,1485p' src/op/copy.cc
echo -e "\n=== Checking variable declarations in src/op/copy.cc ==="
rg -n -B15 'shared_layout.*=' src/op/copy.cc | grep -A2 -B2 'shared_layout' | head -40Length of output: 6418
🏁 Script executed:
#!/bin/bash
# Check what linear_layout is
echo "=== Finding linear_layout definition ==="
rg -n -B5 'linear_layout' src/op/copy.cc | head -30
echo -e "\n=== Finding makeQuarterBankSwizzleLayout return type ==="
rg -n -A3 'makeQuarterBankSwizzleLayout' src/op/copy.cc | head -20
echo -e "\n=== Finding makeQuarterBankSwizzleLayout function definition ==="
rg -n -B2 -A5 'makeQuarterBankSwizzleLayout\s*\(' --type=cpp --type=h | grep -A5 'Layout\|makeQuarterBank'Length of output: 6703
🏁 Script executed:
#!/bin/bash
# Search for all StructuralEqual usage to identify any remaining Layout comparisons
echo "=== All StructuralEqual calls ==="
rg -n 'StructuralEqual\(\)' --type=cpp --type=h | grep -v test | grep -v '\.o:' | head -50
echo -e "\n=== Check if there are any other layout comparisons we missed ==="
rg -n -B2 'shared_layout.*StructuralEqual|StructuralEqual.*shared_layout' src/op/copy.ccLength of output: 3596
Incomplete conversion: Layout comparisons in src/op/copy.cc still use StructuralEqual().
The change at line 162 replaces StructuralEqual() with layout->IsEqual(), but this conversion was not applied consistently across the codebase. Six Layout-to-Layout comparisons in src/op/copy.cc still use the generic StructuralEqual():
- Line 1468:
shared_layoutvslinear_layout - Lines 1477, 1481, 1486, 1491:
shared_layoutvs swizzle layout results (quarter/half/full bank patterns) - Lines 1835, 1839, 1843: similar comparisons in a second function
Update these to use shared_layout->IsEqual(other_layout.get()) for consistency with the specialized equality logic.
🤖 Prompt for AI Agents
In src/transform/layout_inference.cc around lines 162-165 the code was updated
to use layout->IsEqual(...) but similar Layout-to-Layout comparisons in
src/op/copy.cc remain using StructuralEqual(); update each listed comparison to
call the specialized IsEqual on the shared layout. Specifically, replace
occurrences like shared_layout->StructuralEqual(linear_layout.get()) or
shared_layout->StructuralEqual(swizzle_layout.get()) with
shared_layout->IsEqual(linear_layout.get()) and
shared_layout->IsEqual(swizzle_layout.get()) respectively at the noted locations
(around lines 1468, 1477, 1481, 1486, 1491 and around 1835, 1839, 1843); ensure
you call IsEqual on the left-hand shared_layout pointer and pass the other
layout.get() as the argument to preserve semantics.
|
StructuralEqual is too strict for this case: Those two layouts is fully replicated and they can have no conflicts. |
1 participant
This pull request updates the way buffer layout equality is checked in the
BufferUseDefCollectorclass withinsrc/transform/layout_inference.cc. The change replaces the use of the generalStructuralEqualfunction with the more specificIsEqualmethod for layout comparison.Improvement to layout comparison logic:
StructuralEqual()to the more preciselayout->IsEqual()method in theBufferUseDefCollectorclass, ensuring that layout objects are compared using their specialized equality logic.Summary by CodeRabbit