Skip to content

Conversation

@naoyam
Copy link
Collaborator

@naoyam naoyam commented Oct 3, 2025

Hit an error with resize to broadcast while testing #5278. Reject the pattern for now.

@naoyam
Copy link
Collaborator Author

naoyam commented Oct 3, 2025

!test

@github-actions
Copy link

github-actions bot commented Oct 3, 2025

Review updated until commit 7a19d51

Description

  • Reject resize to broadcast in greedy scheduler

  • Add compile-time check for invalid pad patterns

  • Prevent scheduler errors during loop promotion

  • Handle broadcast IDs introduced without BroadcastOp


Changes walkthrough 📝

Relevant files
Bug fix
greedy.cpp
Add rejection of resize-to-broadcast in pad ops                   

csrc/scheduler/greedy.cpp

  • Added check in handle(PadOp*) for resize operations
  • Reject if resize output is a broadcast ID
  • Prevents errors in loop promotion and codegen
  • Addresses unsupported pattern in static scheduling
  • +17/-0   

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🧪 No relevant tests
    ⚡ Recommended focus areas for review

    Possible Issue

    The check for resize->out()->isBroadcast() may not fully capture the intended constraint, as it only inspects the output of the Resize operation. It should be verified whether this correctly identifies cases where a resize is being used to produce a broadcast dimension without an explicit BroadcastOp, or if additional validation is needed on the output tensor's domain.

    for (const auto& logical_id :
         ir_utils::getTvOutput(pad)->getLogicalDomain()) {
      auto resize = dynamic_cast<Resize*>(logical_id->definition());
      if (resize == nullptr) {
        continue;
      }
      // Resize to broadcast not supported yet. Have not looked at
      // details but getLoopPromotion fails at csrc/id_model/utils.h:105 (e.g.,
      // ResizeTest.ResizePadToBroadcastStatic), likely because
      // broadcast IDs are introduced without BroadcastOp.
      // This is also the case with the resize scheduler.
      if (resize->out()->isBroadcast()) {
        reject("Resize to a broadcast ID is not allowed: ", pad->toString());
        return;
      }
    }

    Copy link
    Collaborator

    @jjsjann123 jjsjann123 left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    stamping to unblock.

    But the engineer inside me is screaming for the lack of test and issue for following up with proper fix. :)
    If we have an issue tracking this for the resize scheduler, might be worth to throw a link to the issue in the comment.

    @naoyam
    Copy link
    Collaborator Author

    naoyam commented Oct 10, 2025

    stamping to unblock.

    But the engineer inside me is screaming for the lack of test and issue for following up with proper fix. :) If we have an issue tracking this for the resize scheduler, might be worth to throw a link to the issue in the comment.

    The test is mentioned in the comment (ResizeTest.ResizePadToBroadcastStatic). Added a little more detail. As mentioned there, I haven't looked into this pattern, so I'm not really sure if it's easily fixable or something more fundamental.

    @naoyam
    Copy link
    Collaborator Author

    naoyam commented Oct 10, 2025

    !build

    @naoyam naoyam merged commit ece84c8 into main Oct 10, 2025
    14 of 15 checks passed
    @naoyam naoyam deleted the greedy_pad_static_check branch October 10, 2025 08:43
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    3 participants