[pull] master from tensorflow:master

RELEASE.md

@@ -34,6 +34,13 @@ In `tensorflow/c/experimental/filesystem/filesystem_interface.h`, removed `TF_Tr
     * Exports `__new__` in public API golden files for subclasses of `tuple` (like `tf.io.FixedLenFeature`) to fix false positives during static type checking.>
 * `tf.data`
     * Fixes a bug in `tf.data.Dataset.scan` where the shape of the state returned by `scan_func` was not strictly validated against the initial state.
+*   `tf.image.adjust_contrast`
+
+    *   Registers the missing Python gradient for the `AdjustContrastv2` op, so
+        `tf.image.adjust_contrast` can now be differentiated with
+        `GradientTape`. Fixes
+        [#126083](https://github.com/tensorflow/tensorflow/issues/126083).
+
 *   `tf.experimental.numpy`
 
     *   `tf.experimental.numpy.isclose` and `tf.experimental.numpy.allclose` now

tensorflow/cc/framework/cc_op_gen.cc

@@ -119,7 +119,12 @@ void WriteClassDecl(const OpInfo& op_info, WritableFile* h) {
       const auto entry = AttrTypeName(attr.type());
       const auto attr_type_name = entry.first;
       const bool use_const = entry.second;
-      const std::string camel_case_name = ToCamelCase(api_def_attr.rename_to());
+      // See cc_op_gen_util.cc's GetOpAttrStruct: attr.name() and
+      // api_def_attr.rename_to() are both unvalidated at OpDef-registration
+      // time and both spliced as a raw C++ identifier below.
+      const std::string safe_attr_name =
+          SafeRenameTo(attr.name(), api_def_attr.rename_to());
+      const std::string camel_case_name = ToCamelCase(safe_attr_name);
       const std::string suffix =
           (camel_case_name == op_info.op_name || camel_case_name == "Attrs")
               ? "_"
@@ -212,10 +217,19 @@ std::string GetConstructorBody(const OpInfo& op_info) {
   for (int i = 0; i < op_info.graph_op_def.input_arg_size(); ++i) {
     const auto& arg(op_info.graph_op_def.input_arg(i));
     const auto& api_def_arg(op_info.api_def.in_arg(i));
+    // See cc_op_gen_util.cc's GetOpAttrStruct comment: arg.name() and
+    // api_def_arg.rename_to() are both unvalidated at OpDef-registration
+    // time. Spliced twice below -- once as the local variable name
+    // (`_<name>`), once via AvoidCPPKeywords as the argument reference --
+    // so both must agree with each other and with the corresponding
+    // .Input(_<name>) reference generated further down from the same
+    // (arg.name(), rename_to()) pair.
+    const std::string safe_arg_name =
+        SafeRenameTo(arg.name(), api_def_arg.rename_to());
     strings::StrAppend(
-        &body, "  auto _", api_def_arg.rename_to(), " = ::tensorflow::ops::",
+        &body, "  auto _", safe_arg_name, " = ::tensorflow::ops::",
         ArgIsList(arg) ? "AsNodeOutList" : "AsNodeOut", "(", scope_str, ", ",
-        AvoidCPPKeywords(api_def_arg.rename_to()), ");\n");
+        AvoidCPPKeywords(safe_arg_name), ");\n");
     absl::StrAppend(&body, "  ", return_on_error, "\n");
   }
 
@@ -228,7 +242,11 @@ std::string GetConstructorBody(const OpInfo& op_info) {
   const std::string spaces = "                     ";
   for (int i = 0; i < op_info.api_def.in_arg_size(); ++i) {
     const auto& arg(op_info.api_def.in_arg(i));
-    absl::StrAppend(&body, spaces, ".Input(_", arg.rename_to(), ")\n");
+    // Same (name, rename_to) pair as the declaration loop above, for the
+    // same index -- SafeRenameTo is pure, so this independently produces
+    // the identical identifier the declaration above already emitted.
+    const std::string safe_arg_name = SafeRenameTo(arg.name(), arg.rename_to());
+    absl::StrAppend(&body, spaces, ".Input(_", safe_arg_name, ")\n");
   }
   for (int i = 0; i < op_info.api_def.attr_size(); ++i) {
     const auto& graph_attr(op_info.graph_op_def.attr(i));
@@ -237,12 +255,18 @@ std::string GetConstructorBody(const OpInfo& op_info) {
         op_info.inferred_input_attrs.end()) {
       continue;
     }
+    // See above: graph_attr.name() and api_def_attr.rename_to() are both
+    // unvalidated, and both are spliced as a raw C++ identifier/field
+    // access below.
+    const std::string safe_attr_name =
+        SafeRenameTo(graph_attr.name(), api_def_attr.rename_to());
     const std::string attr_name =
         api_def_attr.has_default_value()
-            ? absl::StrCat("attrs.", api_def_attr.rename_to(), "_")
-            : AvoidCPPKeywords(api_def_attr.rename_to());
-    strings::StrAppend(&body, spaces, ".Attr(\"", graph_attr.name(), "\", ",
-                       attr_name, ")\n");
+            ? absl::StrCat("attrs.", safe_attr_name, "_")
+            : AvoidCPPKeywords(safe_attr_name);
+    strings::StrAppend(&body, spaces, ".Attr(\"",
+                       absl::CEscape(graph_attr.name()), "\", ", attr_name,
+                       ")\n");
   }
   absl::StrAppend(&body, "  ;\n");
   absl::StrAppend(&body, "  ", scope_str, ".UpdateBuilder(&builder);\n");

tensorflow/cc/framework/cc_op_gen_util.cc

@@ -495,6 +495,16 @@ std::string AvoidCPPKeywords(absl::string_view name) {
   return std::string(name);
 }
 
+std::string SafeRenameTo(absl::string_view name, absl::string_view rename_to) {
+  if (tensorflow::IsValidAttrOrArgName(rename_to)) {
+    return std::string(rename_to);
+  }
+  if (tensorflow::IsValidAttrOrArgName(name)) {
+    return std::string(name);
+  }
+  return tensorflow::SanitizeToIdentifier(name);
+}
+
 void InferArgAttributes(
     const OpDef::ArgDef& arg,
     std::unordered_map<std::string, std::string>* inferred_attrs) {
@@ -567,14 +577,24 @@ OpInfo::OpInfo(const OpDef& graph_op_def, const ApiDef& api_def,
     const auto& api_def_arg = *FindInputArg(api_def.arg_order(i), api_def);
     arg_types.push_back(
         absl::StrCat("::tensorflow::", ArgIsList(arg) ? "InputList" : "Input"));
-    arg_names.push_back(AvoidCPPKeywords(api_def_arg.rename_to()));
+    // rename_to() comes from ApiDef, a separate message from the OpDef
+    // arg/attr names. Both are spliced as a raw C++ identifier (parameter
+    // name) below: prefer rename_to() when it's a safe identifier, fall
+    // back to the original arg name when THAT is safe, and only sanitize
+    // as a last resort, since IsValidAttrOrArgName is not enforced at
+    // OpDef registration and a legitimately-registered op's argument name
+    // is not guaranteed to already be a safe identifier (e.g.
+    // TFLite_Detection_PostProcess's "raw_outputs/box_encodings").
+    const std::string safe_input_name =
+        SafeRenameTo(arg.name(), api_def_arg.rename_to());
+    arg_names.push_back(AvoidCPPKeywords(safe_input_name));
 
     // TODO(keveman): Include input type information.
     absl::string_view description = api_def_arg.description();
     if (!description.empty()) {
       ConsumeEquals(&description);
-      absl::StrAppend(&comment, "* ", AvoidCPPKeywords(api_def_arg.rename_to()),
-                      ": ", api_def_arg.description(), "\n");
+      absl::StrAppend(&comment, "* ", AvoidCPPKeywords(safe_input_name), ": ",
+                      api_def_arg.description(), "\n");
     }
   }
 
@@ -593,7 +613,14 @@ OpInfo::OpInfo(const OpDef& graph_op_def, const ApiDef& api_def,
     const auto entry = AttrTypeName(attr.type());
     const auto attr_type_name = entry.first;
     const bool use_const = entry.second;
-    std::string attr_name = AvoidCPPKeywords(api_def_attr.rename_to());
+    // See the safe_input_name comment above: rename_to() and the original
+    // attr name are both unvalidated at OpDef-registration time and both
+    // get spliced as a raw C++ identifier here, so prefer rename_to() when
+    // safe, fall back to the original name when THAT is safe, and only
+    // sanitize as a last resort.
+    const std::string safe_attr_name =
+        SafeRenameTo(attr.name(), api_def_attr.rename_to());
+    std::string attr_name = AvoidCPPKeywords(safe_attr_name);
 
     std::string attr_comment;
     if (!api_def_attr.description().empty()) {
@@ -630,7 +657,10 @@ OpInfo::OpInfo(const OpDef& graph_op_def, const ApiDef& api_def,
     bool is_list = ArgIsList(arg);
     output_types.push_back(
         absl::StrCat("::tensorflow::", is_list ? "OutputList" : "Output"));
-    output_names.push_back(AvoidCPPKeywords(api_def_arg.rename_to()));
+    // See the safe_input_name comment above: the output arg's name and
+    // rename_to() need the same three-tier fallback as the input case.
+    output_names.push_back(
+        AvoidCPPKeywords(SafeRenameTo(arg.name(), api_def_arg.rename_to())));
     is_list_output.push_back(is_list);
   }
 
@@ -694,7 +724,15 @@ std::string OpInfo::GetOpAttrStruct() const {
     const auto entry = AttrTypeName(attr.type());
     const auto attr_type_name = entry.first;
     const bool use_const = entry.second;
-    const std::string camel_case_name = ToCamelCase(api_def_attr.rename_to());
+    // See the safe_input_name comment in OpInfo::OpInfo: attr.name() and
+    // api_def_attr.rename_to() are both unvalidated at OpDef-registration
+    // time, and both are spliced as a raw C++ identifier multiple times
+    // below (the setter name, the field access, the static defaults
+    // function name, and the field declaration) -- computed once here so
+    // every splice site below agrees on the same identifier.
+    const std::string safe_attr_name =
+        SafeRenameTo(attr.name(), api_def_attr.rename_to());
+    const std::string camel_case_name = ToCamelCase(safe_attr_name);
     const std::string suffix =
         (camel_case_name == op_name || camel_case_name == "Attrs") ? "_" : "";
     const std::string attr_func_def =
@@ -713,8 +751,7 @@ std::string OpInfo::GetOpAttrStruct() const {
     absl::StrAppend(&setters, "    TF_MUST_USE_RESULT Attrs ", attr_func_def,
                     " x) {\n");
     absl::StrAppend(&setters, "      Attrs ret = *this;\n");
-    absl::StrAppend(&setters, "      ret.", api_def_attr.rename_to(),
-                    "_ = x;\n");
+    absl::StrAppend(&setters, "      ret.", safe_attr_name, "_ = x;\n");
     absl::StrAppend(&setters, "      return ret;\n    }\n\n");
 
     std::string field_initiliazer;
@@ -724,7 +761,7 @@ std::string OpInfo::GetOpAttrStruct() const {
       // Non-empty lists need static storage for their defaults. Define a
       // function with static local variable that stores the array.
       absl::StrAppend(&defaults_static_storage, "    static ", attr_type_name,
-                      " Default_", api_def_attr.rename_to(), "() {\n");
+                      " Default_", safe_attr_name, "() {\n");
       absl::StrAppend(
           &defaults_static_storage, "      static const ",
           ListElementTypeName(attr.type()), " kStorage[] = ",
@@ -733,14 +770,13 @@ std::string OpInfo::GetOpAttrStruct() const {
       absl::StrAppend(&defaults_static_storage, "      return ", attr_type_name,
                       "(kStorage);\n    }\n");
       // Set the field_initializer to call the defined function.
-      absl::StrAppend(&field_initiliazer, "Default_", api_def_attr.rename_to(),
-                      "()");
+      absl::StrAppend(&field_initiliazer, "Default_", safe_attr_name, "()");
     } else {
       field_initiliazer =
           PrintAttrValue(graph_op_def.name(), api_def_attr.default_value());
     }
-    absl::StrAppend(&struct_fields, "    ", attr_type_name, " ",
-                    api_def_attr.rename_to(), "_ = ", field_initiliazer, ";\n");
+    absl::StrAppend(&struct_fields, "    ", attr_type_name, " ", safe_attr_name,
+                    "_ = ", field_initiliazer, ";\n");
   }
 
   if (struct_fields.empty()) {

tensorflow/cc/framework/cc_op_gen_util.h

@@ -106,6 +106,16 @@ bool IsCPPKeyword(absl::string_view name);
 
 std::string AvoidCPPKeywords(absl::string_view name);
 
+// Returns a name safe to splice as a raw C++ identifier: `rename_to` if it
+// is already a safe identifier (see IsValidAttrOrArgName), else `name` if
+// THAT is safe, else a sanitized fallback (see SanitizeToIdentifier).
+// `rename_to` comes from ApiDef, a separate message from the OpDef arg/attr
+// name `name` is drawn from; neither is validated at OpDef-registration
+// time (see op_def_util.cc for why), so both must be checked here rather
+// than trusted, at every place either is spliced as a raw C++ identifier
+// into generated source.
+std::string SafeRenameTo(absl::string_view name, absl::string_view rename_to);
+
 void InferArgAttributes(
     const OpDef::ArgDef& arg,
     std::unordered_map<std::string, std::string>* inferred_attrs);

tensorflow/compiler/jit/BUILD

@@ -1139,7 +1139,6 @@ cc_library(
         "cluster_scoping_pass.cc",
         "deadness_analysis.cc",
         "deadness_analysis_internal.h",
-        "disable_functional_ops_lowering_for_xla_pass.cc",
         "encapsulate_subgraphs_pass.cc",
         "encapsulate_xla_computations_pass.cc",
         "extract_outside_compilation_pass.cc",
@@ -1155,7 +1154,6 @@ cc_library(
         "clone_constants_for_better_clustering.h",
         "cluster_scoping_pass.h",
         "deadness_analysis.h",
-        "disable_functional_ops_lowering_for_xla_pass.h",
         "encapsulate_subgraphs_pass.h",
         "encapsulate_xla_computations_pass.h",
         "extract_outside_compilation_pass.h",
@@ -1332,7 +1330,6 @@ tf_cc_test(
         "build_xla_ops_pass_test.cc",
         "clone_constants_for_better_clustering_test.cc",
         "cluster_scoping_pass_test.cc",
-        "disable_functional_ops_lowering_for_xla_pass_test.cc",
         "encapsulate_subgraphs_pass_test.cc",
         "encapsulate_xla_computations_pass_test.cc",
         "extract_outside_compilation_pass_test.cc",

tensorflow/compiler/jit/compilability_check_util.cc

@@ -354,6 +354,7 @@ bool RecursiveCompilabilityChecker::OpIsInaccurate(const Node& node) const {
 bool RecursiveCompilabilityChecker::OpIsSlow(const Node& node) const {
   // b/128001705: SelfAdjointEigV2 and Svd performance issues.
   // b/135640736: MatrixInverse performance issues.
+  // MatrixDeterminant and LogMatrixDeterminant use the same QR path as Inverse.
   // b/111271662: MatrixSolve performance issues.
   // https://github.com/tensorflow/tensorflow/pull/31012:
   //    ResizeNearestNeighbor, ResizeBilinear, and ResizeBilinearGrad sometimes
@@ -364,6 +365,8 @@ bool RecursiveCompilabilityChecker::OpIsSlow(const Node& node) const {
   return node.type_string() == "SelfAdjointEigV2" ||
          node.type_string() == "Svd" || node.type_string() == "Qr" ||
          node.type_string() == "MatrixInverse" ||
+         node.type_string() == "MatrixDeterminant" ||
+         node.type_string() == "LogMatrixDeterminant" ||
          node.type_string() == "MatrixSolve" ||
          node.type_string() == "ResizeBilinearGrad" ||
          node.type_string() == "NonMaxSuppressionV3" ||