[MLIR][Affine] Switch default option for affine data copy generate pass

[bondhugula]

The more common use of this pass for testing and other purposes is the
pointwise copy generation as opposed to DMA generation. Switch the
default pass option to pointwise copy generation for easier testing. NFC
otherwise.

Users who were relying on generate-dma to be on by default will have
to pass it explicitly: -affine-data-copy-generate='generate-dma=1'.

[llvmbot]

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-affine

Author: Uday Bondhugula (bondhugula)

Changes

The more common use of this pass for testing and other purposes is the
pointwise copy generation as opposed to DMA generation. Switch the
default pass option to pointwise copy generation for easier testing. NFC
otherwise.

---

Full diff: https://github.com/llvm/llvm-project/pull/129445.diff

4 Files Affected:

• (modified) mlir/include/mlir/Dialect/Affine/Passes.td (+1-1)
• (modified) mlir/lib/Dialect/Affine/Transforms/AffineDataCopyGeneration.cpp (+1-1)
• (modified) mlir/test/Dialect/Affine/affine-data-copy.mlir (+2-2)
• (modified) mlir/test/Dialect/Affine/dma-generate.mlir (+2-2)

diff --git a/mlir/include/mlir/Dialect/Affine/Passes.td b/mlir/include/mlir/Dialect/Affine/Passes.td
index c49db0af55aab..0b8d5b7d94861 100644
--- a/mlir/include/mlir/Dialect/Affine/Passes.td
+++ b/mlir/include/mlir/Dialect/Affine/Passes.td
@@ -27,7 +27,7 @@ def AffineDataCopyGeneration : Pass<"affine-data-copy-generate", "func::FuncOp">
            /*default=*/"1",
            "Fast memory space identifier for copy generation (default: 1)">,
     Option<"generateDma", "generate-dma", "bool",
-           /*default=*/"true", "Generate DMA instead of point-wise copy">,
+           /*default=*/"false", "Generate DMA instead of point-wise copy">,
     Option<"minDmaTransferSize", "min-dma-transfer", "int",
            /*default=*/"1024",
            "Minimum DMA transfer size supported by the target in bytes">,
diff --git a/mlir/lib/Dialect/Affine/Transforms/AffineDataCopyGeneration.cpp b/mlir/lib/Dialect/Affine/Transforms/AffineDataCopyGeneration.cpp
index 9ffe54f61ebbd..4d30213cc6ec2 100644
--- a/mlir/lib/Dialect/Affine/Transforms/AffineDataCopyGeneration.cpp
+++ b/mlir/lib/Dialect/Affine/Transforms/AffineDataCopyGeneration.cpp
@@ -86,7 +86,7 @@ struct AffineDataCopyGeneration
 
 /// Generates copies for memref's living in 'slowMemorySpace' into newly created
 /// buffers in 'fastMemorySpace', and replaces memory operations to the former
-/// by the latter. Only load op's handled for now.
+/// by the latter.
 std::unique_ptr<OperationPass<func::FuncOp>>
 mlir::affine::createAffineDataCopyGenerationPass(
     unsigned slowMemorySpace, unsigned fastMemorySpace, unsigned tagMemorySpace,
diff --git a/mlir/test/Dialect/Affine/affine-data-copy.mlir b/mlir/test/Dialect/Affine/affine-data-copy.mlir
index 453a0eabc4fdd..1f3aac3401bea 100644
--- a/mlir/test/Dialect/Affine/affine-data-copy.mlir
+++ b/mlir/test/Dialect/Affine/affine-data-copy.mlir
@@ -1,6 +1,6 @@
-// RUN: mlir-opt %s -split-input-file -affine-data-copy-generate="generate-dma=false fast-mem-space=0 skip-non-unit-stride-loops" | FileCheck %s
+// RUN: mlir-opt %s -split-input-file -affine-data-copy-generate="fast-mem-space=0 skip-non-unit-stride-loops" | FileCheck %s
 // Small buffer size to trigger fine copies.
-// RUN: mlir-opt %s -split-input-file -affine-data-copy-generate="generate-dma=false fast-mem-space=0 fast-mem-capacity=1" | FileCheck --check-prefix=CHECK-SMALL %s
+// RUN: mlir-opt %s -split-input-file -affine-data-copy-generate="fast-mem-space=0 fast-mem-capacity=1" | FileCheck --check-prefix=CHECK-SMALL %s
 
 // Test affine data copy with a memref filter. We use a test pass that invokes
 // affine data copy utility on the input loop nest.
diff --git a/mlir/test/Dialect/Affine/dma-generate.mlir b/mlir/test/Dialect/Affine/dma-generate.mlir
index b38bf896e78cf..62011681091cc 100644
--- a/mlir/test/Dialect/Affine/dma-generate.mlir
+++ b/mlir/test/Dialect/Affine/dma-generate.mlir
@@ -1,5 +1,5 @@
-// RUN: mlir-opt -allow-unregistered-dialect %s -split-input-file -affine-data-copy-generate="generate-dma fast-mem-space=2 skip-non-unit-stride-loops" -verify-diagnostics | FileCheck %s
-// RUN: mlir-opt -allow-unregistered-dialect %s -split-input-file -affine-data-copy-generate="generate-dma fast-mem-capacity=16 fast-mem-space=2" | FileCheck %s --check-prefix FAST-MEM-16KB
+// RUN: mlir-opt -allow-unregistered-dialect %s -split-input-file -affine-data-copy-generate="generate-dma=1 fast-mem-space=2 skip-non-unit-stride-loops" -verify-diagnostics | FileCheck %s
+// RUN: mlir-opt -allow-unregistered-dialect %s -split-input-file -affine-data-copy-generate="generate-dma=1 fast-mem-capacity=16 fast-mem-space=2" | FileCheck %s --check-prefix FAST-MEM-16KB
 
 // We run most test cases with -copy-skip-non-unit-stride-loops to allow testing
 // DMA generation at inner levels easily - since the DMA generation would

[Lewuathe]

I think changing the default behavior can have a significant impact on any downstream projects using this pass. I don't think we necessarily need to change the default value as long as we provide the option.

If we really need to change the default value, it would be great to start the discussion in the community in Discouse so that we can get some feedback from the users of this library.

[bondhugula]

I think changing the default behavior can have a significant impact on any downstream projects using this pass. I don't think we necessarily need to change the default value as long as we provide the option.

If we really need to change the default value, it would be great to start the discussion in the community in Discouse so that we can get some feedback from the users of this library.

Impact on downstream projects?! You just have to set generate-dma=0 if you were relying on the default being true. This is considered nearly zero impact and absolutely trivial, rather than any real or functional! There's no change in actual functionality, but a change in the pass option. For example, renaming a pass option has the same impact as this and doesn't warrant a discourse discussion.

As mentioned in the commit summary, the majority of the testing of this pass happens with the pointwise data copy and not DMA generation - so it's convenient to have the dominant pass option as the default.

[bondhugula]

I think changing the default behavior can have a significant impact on any downstream projects using this pass. I don't think we necessarily need to change the default value as long as we provide the option.
If we really need to change the default value, it would be great to start the discussion in the community in Discouse so that we can get some feedback from the users of this library.

Impact on downstream projects?! You just have to set generate-dma=0 if you were relying on the default being true. This is considered nearly zero impact and absolutely trivial, rather than any real or functional impact! You get more impact when you pull a new commit from upstream! There's no change in actual functionality but a change in pass option. For example, renaming a pass option has the same impact as this and doesn't warrant a discourse discussion.

As mentioned in the commit summary, the majority of the testing of this pass happens with the pointwise data copy and not DMA generation - so it's convenient to have the dominant pass option as the default.

Although obvious, I've now anyway added a line in the commit summary mentioning the pass option to explicitly set.