Fix issues with generalized_scatter and setitem allocated unbacked symbols.

[laithsakka]

Stack from ghstack (oldest at bottom):

• set unbacked bindings in reinplace pass for newly created nodes during generalize_scatter decomp #164948
• -> Fix issues with generalized_scatter and setitem allocated unbacked symbols. #164341

Three fixes:

1. When doing t[u0] +=1 if u0 is unbacked we could allocate a new unbacked symbol during the the indexing of t[u0] (when we fake trace setitem), namely because meta_select does allocate a new unbacked symbol for the storage offset when we do not know if u0>=0 or u0<0. but the output size/stride of setitem(), does not depend on that new symbol. it's self consumed in setitem so we shall ignore it.

2. Also when we trace through generalized_scatter the applications of the views could allocate unbacked symints
   but those do not effect final output, we also shall ignore them.

3.Before accessing strides in lowering we shall materialize.

Address #114293 and #131911

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @ipiszy @chenyang78 @kadeng @muchulee8 @amjames @chauhang @aakhundov @coconutruben @Lucaskabela

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/164341

• 📄 Preview Python docs built from this PR
• 📄 Preview C++ docs built from this PR
• ❓ Need help or want to give feedback on the CI? Visit the bot commands wiki

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 95f2e2d with merge base 219fb6a ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[laithsakka]

I don't understand the pr description for (1). What does it mean that you get a new unbacked symbol if x is unbacked?

@ezyang
if we have
u0 = ...
and then have
t[u0] +=1

during the meta_select we allocate new unbacked symint to represent the storage offset in the output of the select op. (because we do not know if u0>=0 or u0<0)
the problem is that when we trace setitem its meta call meta_select, but the final output of setitem does not use that symbol.

updated summary.

[ezyang]

I'm also confused here. You are going to have view.target inside of the graph now right? So tmp DOES have the unbacked symbol, and you should resolve unbacked symbols here (instead of suppressing them)

[laithsakka]

this is Not the function that does the de-composition ,
this is the function used to trace the _generalized_scatter.

for the function that does the decomposition we need to do what you said right which i am doing on the PR that is right on top of this one on the stack.
#164948

for this function we self consume the views but return return inp on line 98 which would not have a reference to the new symbols in its example input.

[ezyang]

vc me if you don't agree

[laithsakka]

We discussed this offline for
set_item, we discussed that the alternative approach is to decompose the set_item during dynamo tracing by writing a pollyfill that mimic the logic of this function. I will do that in a different PR since it requires careful implementation.

inline void set_item(
    const Tensor& self,
    const ArrayRef<TensorIndex>& indices,
    const Tensor& value,
    bool disable_slice_optimization = false) {
  at::Device self_device = self.device();
  SymIntArrayRef self_sizes = self.sym_sizes();

  // handle simple types: integers, slices, ellipsis, bool
  if (indices.size() == 1) {
    const TensorIndex& index = indices[0];
    if (index.is_boolean() && !index.boolean()) {
      // do nothing for false (technically we should check the size, but we
      // don't have real 0-sized shapes.
      return;
    } else if (index.is_ellipsis()) {
      copy_to(self, value);
      return;
    } else if (index.is_none() || (index.is_boolean() && index.boolean())) {
      copy_to(self.unsqueeze(0), value);
      return;
    } else if (index.is_integer()) {
      copy_to(
          impl::applySelect(
              self, 0, index.integer(), 0, self_device, self_sizes),
          value);
      return;
    } else if (index.is_slice()) {
      copy_to(
          impl::applySlice(
              self,
              0,
              index.slice().start(),
              index.slice().stop(),
              index.slice().step(),
              /*disable_slice_optimization=*/disable_slice_optimization,
              self_device,
              self_sizes),
          value);
      return;
    }
  }

  std::vector<Tensor> tensorIndices;
  Tensor sliced = impl::applySlicing(
      self,
      indices,
      tensorIndices,
      disable_slice_optimization,
      self_device,
      self_sizes);
  if (tensorIndices.empty()) {
    copy_to(sliced, value);
    return;
  }

  SymIntArrayRef valueSizes = value.sym_sizes();
  SymIntArrayRef slicedValueSizes = slicePrefix1sSize(valueSizes);
  Tensor valuesSliced;
  if (!valueSizes.equals(slicedValueSizes)) {
    valuesSliced = value.view_symint(slicedValueSizes);
  } else {
    valuesSliced = value;
  }
  dispatch_index_put_(sliced, std::move(tensorIndices), valuesSliced);
  return;
}

for the generalized_scatter the current approach is fine @bobrenjc93 is looking at this problem more generally for High order ops.

[bobrenjc93]

s/unabacked/unbacked

[bobrenjc93]

add a comment?

[bobrenjc93]

ditto

[pytorchmergebot]

Starting merge as part of PR stack under #164948

[laithsakka]

address comments and rebase

[laithsakka]

@pytorchmergebot merge

[pytorch-bot]

This PR has pending changes requested. Please address the comments and update the PR before merging.

[pytorchmergebot]

Starting merge as part of PR stack under #164948