Use kUnknownDimFromConst for output_tensors_as_shapes for Pack op.

Without this, Const -1 input to Pack may be used in different ops,
where SymblociShapeManager may annotate unknown shapes, which can cause
wrong graph optimization (especially, common subgraph ellimination,
combined with constant folding).

PiperOrigin-RevId: 351181242
Change-Id: I5baea7108a97345e9e6cf977ed5fb507013bd15f

tensorflow/core/grappler/costs/graph_properties.cc

@@ -1749,7 +1749,8 @@ class SymbolicShapeRefiner {
             }
             int64 size = t->dtype() == DT_INT32 ? t->scalar<int32>()()
                                                 : t->scalar<int64>()();
-            dims.push_back(size < 0 ? ic->UnknownDim() : ic->MakeDim(size));
+            dims.push_back(size < 0 ? ic->MakeDim(kUnknownDimFromConst)
+                                    : ic->MakeDim(size));
           } else {
             // Don't have tensor value, but use input_tensors_as_shapes, if
             // possible.
@@ -1759,7 +1760,9 @@ class SymbolicShapeRefiner {
                 ic->ValueKnown(ic->Dim(shape_handle, 0))) {
               dims.push_back(ic->Dim(shape_handle, 0));
             } else {
-              dims.push_back(ic->UnknownDim());
+              // This is not from Const, but as it shouldn'be used as symbolic
+              // unknown dim for different ops, we use kUnknownDimFromConst.
+              dims.push_back(ic->MakeDim(kUnknownDimFromConst));
             }
           }
         }

tensorflow/core/grappler/costs/graph_properties_test.cc

@@ -1128,6 +1128,88 @@ TEST_F(GraphPropertiesTest, SizeOp) {
   ExpectTensorValues({24}, identity_props0.value());
 }
 
+TEST_F(GraphPropertiesTest, PackWithConstMinus1AndReshapes) {
+  tensorflow::Scope s = tensorflow::Scope::NewRootScope();
+  Output shape0 = ops::Const(s.WithOpName("shape0"), 4, {});
+  Output shape1 = ops::Const(s.WithOpName("shape1"), -1, {});
+  Output pack = ops::Stack(s.WithOpName("pack"), {shape0, shape1});
+  // pack is [2], with values {4, -1}.
+
+  Output x0_ = ops::Placeholder(s.WithOpName("x0_"), DataType::DT_FLOAT);
+  Output x1_ = ops::Placeholder(s.WithOpName("x1_"), DataType::DT_FLOAT);
+
+  Output x0 = ops::Reshape(s.WithOpName("x0"), x0_, pack);
+  Output x1 = ops::Reshape(s.WithOpName("x1"), x1_, pack);
+  // Two unknown rank tensors (x0_ and x1_) are reshaped with pack {4, -1},
+  // their output shapes would be [4, -1]. However, though we use the same
+  // shape input to the Reshape ops, their output shapes can be different;
+  // i.e., unknown dim values (-1) of x0 and x1 shapes are not necessarily
+  // the same.
+
+  // if input to the Select ops. Note that s0 has a fully defined shape, while
+  // s1 has unknown shape.
+  Output s0 = ops::Const(s.WithOpName("s0"), true, {4, 16});
+  Output s1 = ops::Placeholder(s.WithOpName("s1"), DataType::DT_BOOL);
+
+  Output y0 = ops::Placeholder(s.WithOpName("y0"), DataType::DT_FLOAT);
+  Output y1 = ops::Placeholder(s.WithOpName("y1"), DataType::DT_FLOAT);
+
+  // We instantiate SelectV2, but will replace it with Select. The shape
+  // inference function for Select links all inputs and outputs as they should
+  // have the same shapes.
+  Output z0 = ops::SelectV2(s.WithOpName("z0"), s0, x0, y0);
+  Output z1 = ops::SelectV2(s.WithOpName("z1"), s1, x1, y1);
+
+  // For z0, as we know the shape of s0, symbolic shape manager in shape
+  // inference will make the shapes of x0, y0, and z0 equal to the shape of s0,
+  // which is [4, 16].
+  // For z1, s0 and y1 are all unknown shapes, so we can infer they're [4, -1]
+  // at best.
+  // Note that x0 and x1 share the same shape input to the Reshape op, but
+  // -1 in the shape input should not be treated as the same symoblic unknown
+  // dim; it is merely a constant value -1 for identitying unknown dim for
+  // Reshape operation.
+
+  GrapplerItem item;
+  TF_ASSERT_OK(s.ToGraphDef(&item.graph));
+
+  // Replace SelectV2 op with Select op.
+  for (int i = 0; i < item.graph.node_size(); ++i) {
+    auto* node = item.graph.mutable_node(i);
+    if (node->op() == "SelectV2") {
+      node->set_op("Select");
+    }
+  }
+
+  GraphProperties properties(item);
+  TF_ASSERT_OK(properties.InferStatically(false));
+  for (const auto& node_name : {"x0", "y0", "z0"}) {
+    const auto out_props = properties.GetOutputProperties(node_name);
+    const OpInfo::TensorProperties out_prop0 = out_props[0];
+    EXPECT_EQ("float: [4,16]", PropToString(out_prop0));
+  }
+  {
+    const auto out_props = properties.GetOutputProperties("s0");
+    const OpInfo::TensorProperties out_prop0 = out_props[0];
+    EXPECT_EQ("bool: [4,16]", PropToString(out_prop0));
+  }
+
+  for (const auto& node_name : {"x1", "y1", "z1"}) {
+    const auto out_props = properties.GetOutputProperties(node_name);
+    const OpInfo::TensorProperties out_prop0 = out_props[0];
+    EXPECT_EQ("float: [4,-1]", PropToString(out_prop0));
+  }
+  // if input of Select can be either vector or the same shape to the
+  // input/output; in this case, even if we know input and output are
+  // [4, ?], we can't say it's [4, ?] or a vector; hence, it shoudl be
+  // unknown.
+  {
+    const auto out_props = properties.GetOutputProperties("s1");
+    const OpInfo::TensorProperties out_prop0 = out_props[0];
+    EXPECT_EQ("bool: ?", PropToString(out_prop0));
+  }
+}
+
 TEST_F(GraphPropertiesTest, PackWithIdentityInput) {
   tensorflow::Scope s = tensorflow::Scope::NewRootScope();
   // Same to PackWithConstInput test case, but a, b, c, and d are Identity ops