From 583b7418a6a38f8e5c7b1c442d8642ce26cc5469 Mon Sep 17 00:00:00 2001
From: Adrian Kuegel <akuegel@google.com>
Date: Wed, 8 Jan 2020 00:39:25 -0800
Subject: [PATCH] Fix confusing way of specifying filter_expansion parameter.

Currently, the constructor of ConvolutionGroupConverter allows to specify
canonicalize_depthwise_filter without specifying what it does (assigning it
to a variable called filter_expansion). Then this is used to initialize the
filter_expansion variable of visitor with !filter_expansion. So essentially
one needed to pass 'false' if the filter expansion should be done.
This CL makes it clearer by using always the variable name filter_expansion
and avoids the intermediate negation step.
No functional change.

PiperOrigin-RevId: 288647349
Change-Id: I0f2984f403d6b4e0cc88405bd05234cb6a7b8a92
---
 .../xla/service/convolution_group_converter.cc      | 13 +++++--------
 .../xla/service/convolution_group_converter.h       |  4 ++--
 tensorflow/compiler/xla/service/gpu/gpu_compiler.cc |  2 +-
 3 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/tensorflow/compiler/xla/service/convolution_group_converter.cc b/tensorflow/compiler/xla/service/convolution_group_converter.cc
index 06bcd773f44..9ecadbf6c82 100644
--- a/tensorflow/compiler/xla/service/convolution_group_converter.cc
+++ b/tensorflow/compiler/xla/service/convolution_group_converter.cc
@@ -56,8 +56,7 @@ class ConvolutionVisitor : public DfsHloVisitorWithDefault {
   // Runs the visitor on a computation.
   static bool Run(HloComputation* computation,
                   std::function<bool(HloInstruction*)> is_cost_viable,
-                  bool convert_batch_groups_only,
-                  bool canonicalize_depthwise_filter);
+                  bool convert_batch_groups_only, bool filter_expansion);
 
   // Returns whether any convolution ops were rewritten.
   const bool changed() const { return changed_; }
@@ -68,10 +67,9 @@ class ConvolutionVisitor : public DfsHloVisitorWithDefault {
   explicit ConvolutionVisitor(
       HloComputation* computation,
       std::function<bool(HloInstruction*)> is_cost_viable,
-      bool convert_batch_groups_only,
-      bool canonicalize_depthwise_filter = false)
+      bool convert_batch_groups_only, bool filter_expansion)
       : computation_(computation),
-        filter_expansion_(!canonicalize_depthwise_filter),
+        filter_expansion_(filter_expansion),
         convert_batch_groups_only_(convert_batch_groups_only),
         is_cost_viable_(is_cost_viable) {}
 
@@ -94,10 +92,9 @@ class ConvolutionVisitor : public DfsHloVisitorWithDefault {
 bool ConvolutionVisitor::Run(
     HloComputation* computation,
     std::function<bool(HloInstruction*)> is_cost_viable,
-    bool convert_batch_groups_only, bool canonicalize_depthwise_filter) {
+    bool convert_batch_groups_only, bool filter_expansion) {
   ConvolutionVisitor visitor(computation, is_cost_viable,
-                             convert_batch_groups_only,
-                             canonicalize_depthwise_filter);
+                             convert_batch_groups_only, filter_expansion);
   TF_CHECK_OK(computation->Accept(&visitor));
   return visitor.changed_;
 }
diff --git a/tensorflow/compiler/xla/service/convolution_group_converter.h b/tensorflow/compiler/xla/service/convolution_group_converter.h
index 1caf1841119..a8a91ed1018 100644
--- a/tensorflow/compiler/xla/service/convolution_group_converter.h
+++ b/tensorflow/compiler/xla/service/convolution_group_converter.h
@@ -29,10 +29,10 @@ class ConvolutionGroupConverter : public HloModulePass {
  public:
   ConvolutionGroupConverter(std::function<bool(HloInstruction*)> is_cost_viable,
                             bool convert_batch_groups_only,
-                            bool canonicalize_depthwise_filter = false)
+                            bool filter_expansion = true)
       : is_cost_viable_(is_cost_viable),
         convert_batch_groups_only_(convert_batch_groups_only),
-        filter_expansion_(canonicalize_depthwise_filter) {}
+        filter_expansion_(filter_expansion) {}
 
   absl::string_view name() const override {
     return "convolution-group-converter";
diff --git a/tensorflow/compiler/xla/service/gpu/gpu_compiler.cc b/tensorflow/compiler/xla/service/gpu/gpu_compiler.cc
index 04761123127..59260a8217a 100644
--- a/tensorflow/compiler/xla/service/gpu/gpu_compiler.cc
+++ b/tensorflow/compiler/xla/service/gpu/gpu_compiler.cc
@@ -152,7 +152,7 @@ Status GpuCompiler::OptimizeHloModule(
     pipeline.AddPass<ConvolutionGroupConverter>(
         batch_group_cost_model,
         /*convert_batch_groups_only=*/true,
-        /*canonicalize_depthwise_filter=*/false);
+        /*filter_expansion=*/true);
 
     auto cost_model = [](HloInstruction* conv) {
       // We need a cost model for GPUs. Currently, do nothing.