preseg optimization pass - merge neighboring PadOp#2490
preseg optimization pass - merge neighboring PadOp#2490jjsjann123 merged 0 commit intopreseg_propagate_padfrom
PadOp#2490Conversation
jjsjann123
changed the title
jjsjann123
force-pushed
the
preseg_propagate_pad_merge_pad
branch
from
f27a1b5 to
b2fca30
Compare
jjsjann123
changed the title
PadOp
jjsjann123
requested review from
jacobhinkle,
kevinstephano,
naoyam,
wujingyue and
zasdfgbnm
| if (pad_out->uses().size() != 1 || !pad_out->uses()[0]->isA<PadOp>()) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
I think we might also need to prove that each pad width has the same sign in producer and consumer, or at least that the producer sign is positive. Otherwise, we might have something like the following:
auto tv1 = pad(tv0, {0, -1}, 0);
auto tv2 = pad(tv1, {0, 1}, 0); // same as tv0 with last element replaced by 0
If we're not careful here we would replace this with a pad by {0, 0}. This simple example could sneak by us when using symbolic pad widths.
There was a problem hiding this comment.
Thanks for pointing this out (as well as the comment here). I was fooled by the comment #2373 (comment).
auto tv1 = pad(tv0, {0, -1}, 0);
auto tv2 = pad(tv1, {0, 1}, 0); // same as tv0 with last element replaced by 0
I'm wondering if we can still merge the two pads even with symbolic pad widths. i.e. shouldn't concretization have already inserted logic to ensure that runtime value isn't going to change the semantics? I'll try to chat with you offline on this one.
There was a problem hiding this comment.
Hmm, but we don't have a single op that can set just the last element to 0, so what would the merged case be in this case? I agree that we can handle symbolic pad widths I think, it just might need some help. You're right that this could potentially become a concretization thing, similar to #511 .
There was a problem hiding this comment.
but we don't have a single op that can set just the last element to 0
oh sorry I misread that earlier. It's the shrink then pad which alters the value in the output. Yeah we cannot do that. But we would be able to merge the other direction. I'll fix it up.
Thanks a ton for the example. 🙇
|
in the later refactor, it no longer makes sense to keep this PR separate. But I did address @jacobhinkle's comment in the original PR. |
2 participants
multiple
catwould ended up having theirPadOpbeing pushed to the same spot giving us a chain ofPadOp. The added fusion ir pass tries to merge neighboringPadOpas one.