Fixing DomainMap::areAllTargetIdsCoveredBy to handle broadcast to non-root IDs
#3655
Conversation
|
!test |
| if (!getMappedInputConcreteID(covered_source_ids, concrete_source_id_out)) { | ||
| return false; | ||
|
|
||
| // After mapping with PERMISSIVE map, `concrete_id_out` might no longer be a |
There was a problem hiding this comment.
This is the patch. I added another call to get_source_iter_domain({concrete_id_out})
There was a problem hiding this comment.
So, for a given logical ID, we take its source IDs, then for each source ID, take the source IDs of its concrete ID. I understand it would work for the repro, but I wonder why it would need to take source IDs twice. For example, would it work if we took first the concrete ID of a logical ID and then its source IDs?
There was a problem hiding this comment.
I need to take the first source ID of the logical ID for the pad example to work, i.e. in the comment above from line 252 - 258.
T2[i0, i1] = T0[i0, b0] + T1[i0, i1]
T3[i0, i9] = pad(T0[i0, b0])
We want to have T2 as the reference TV and we want to be able to map T3 to T2. For i9 in T3. I wanted to first map to source ID, so it can be map to b0, which would concretize to i1.
So my naive justification of going through source IDs twice is that: the first source ID call allows us to trace back to broadcast IDs that went through a resize; while the second source ID calls allow us to resolve transformations on the concrete IDs.
There was a problem hiding this comment.
I'm fine with this PR as long as #3653 is fixed, but I'm feeling there's something not quite right here, probably because T2 and T3 shouldn't be actually mapped. They could have completely different sizes, and neither T2 nor T3 should not be considered representative.
There was a problem hiding this comment.
I agree with you.
We have some existing tests that asserts the behavior, which might not be what we would want to support moving forward.
I'll update the comment here per suggestion and merge this PR as-is just so we can patch the issue.
Meanwhile, I think the case where you are making is that, we shouldn't map through PERMISSIVE mapping in the first place here. I'll start a draft PR with that to see how many tests are affected. We can discuss what's the next step then.
There was a problem hiding this comment.
I'm not sure if it's something we should worry about at this moment. No test should fail functionally since it's a segmentation issue. It would be just a performance difference, but unless we have a benchmark that has this particular pattern, it doesn't make much sense to discuss what we should do.
There was a problem hiding this comment.
Thanks for fixing the issue. Could you add some more notes about the logic as you mentioned here: #3655 (comment)
|
!test |
Fixes #3653
This PR adds another projection to source ID after permissive mapping. This allows us to correctly identify coverage when the broadcast IDs are not source ID directly.