Skip to content

Comments

[DO NOT REVIEW] see if it blows up#4310

Closed
jjsjann123 wants to merge 52 commits intomainfrom
jj/embedding_ir
Closed

[DO NOT REVIEW] see if it blows up#4310
jjsjann123 wants to merge 52 commits intomainfrom
jj/embedding_ir

Conversation

@jjsjann123
Copy link
Collaborator

No description provided.

@github-actions
Copy link

github-actions bot commented Apr 24, 2025

Review updated until commit 0146b7c

Description

  • Added support for ScatterOp in various parts of the codebase.

  • Modified TensorDomain to include no_loop_ids for better handling of non-loop domains.

  • Updated getPredicateDomains to consider ScatterOp for predicate domains.

  • Enhanced getConsumerAllocationIndices and getGlobalConsumerStridedIndices to handle override indices.


Changes walkthrough 📝

Relevant files
Enhancement
18 files
fusion_segmenter.cpp
Removed assertion on loop domain for inputs.                         
+6/-5     
indexing.cpp
Removed unsupported indirect indexing check.                         
+0/-5     
predicate_indexing.cpp
Updated predicate domains for ScatterOp.                                 
+13/-1   
index_compute.cpp
Added override_index parameter for allocation indices.     
+11/-4   
nodes.cpp
Added methods for ScatterOp and updated TensorDomain validation.
+35/-12 
utils.cpp
Added ScatterOp checks and updated root-to-loop validation.
+34/-6   
logical_domain_map.cpp
Updated non-mapping domain info for ScatterOp.                     
+34/-9   
mutator.cpp
Updated OptOutMutator to handle `no_loop_ids`.                     
+3/-1     
indexing.cpp
Updated scatterOp to include root and loop domains.           
+21/-8   
pointwise.cpp
Moved loop domain retrieval after error check.                     
+1/-2     
registry_utils.cpp
Added ScatterOp checks for fusion output requirement.       
+19/-0   
domain_map.cpp
Updated input ID mapping check to use loop domain.             
+7/-1     
utils.cpp
Added ScatterOp checks for caching and vectorization.       
+6/-1     
index_compute.h
Updated getConsumerAllocationIndices to include override_index.
+2/-1     
internal_base_nodes.h
Added `no_loop_ids` to TensorDomain.                                         
+10/-1   
internal_nodes.h
Added methods for ScatterOp ID retrieval.                               
+4/-0     
utils.h
Added ScatterOp checks and updated hasRootToLoopLinearTransformations.
+4/-1     
logical_domain_map.h
Added ScatterOp handling in ComputeAtLogicalDomainMapBuilder.
+4/-0     
Tests
1 files
test_scatter.cpp
Added tests for ScatterOp compilation and manual inplace operations.
+103/-0 

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests
⚡ Recommended focus areas for review

Code Comment

The comment about not asserting on loop domain of inputs should be clarified or removed if it's no longer relevant.

// we shouldn't assert on loopdomain of inputs, because they carry no meaning.
// NVF_ERROR(
//     tv->getLoopDomain() == tv->getLogicalDomain(),
//     tv,
//     " has an unexpected loop domain:\n",
Code Comment

The comment about categorizing predicate on root should be clarified or removed if it's no longer relevant.

// NOTE: we should categories this as predicate on root, I think reduction is similar to how scatter is being represented
std::vector<IterDomain*> predicate_domains = (consumer_tv->hasReduction() || expr->isA<ScatterOp>())
Code Comment

The comment about the FIXME in the scatter operation should be addressed or removed if it's no longer relevant.

}

IterDomain* SelectOp::getIndexedID() const {

@jjsjann123
Copy link
Collaborator Author

!test

@jjsjann123
Copy link
Collaborator Author

!test

@jjsjann123
Copy link
Collaborator Author

I'm really surprised that this one worked......

@jjsjann123
Copy link
Collaborator Author

closing smoke test.

@jjsjann123 jjsjann123 closed this Aug 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant