Scalarizer: Replace cl::opts with pass parameters (llvm#110645)

Preserve the existing defaults (although load-store defaulting to false is a really bad one). Also migrate DirectX tests to new PM.
ROCm · Oct 2, 2024 · 1bc9b67 · 1bc9b67
1 parent 4f6ad17
commit 1bc9b67
Show file tree

Hide file tree

Showing 23 changed files with 93 additions and 64 deletions.
diff --git a/llvm/include/llvm/Transforms/Scalar/Scalarizer.h b/llvm/include/llvm/Transforms/Scalar/Scalarizer.h
@@ -27,13 +27,25 @@ class Function;
 class FunctionPass;
 
 struct ScalarizerPassOptions {
-  // These options correspond 1:1 to cl::opt options defined in
-  // Scalarizer.cpp. When the cl::opt are specified, they take precedence.
-  // When the cl::opt are not specified, the present optional values allow to
-  // override the cl::opt's default values.
-  std::optional<bool> ScalarizeVariableInsertExtract;
-  std::optional<bool> ScalarizeLoadStore;
-  std::optional<unsigned> ScalarizeMinBits;
+  /// Instruct the scalarizer pass to attempt to keep values of a minimum number
+  /// of bits.
+
+  /// Split vectors larger than this size into fragments, where each fragment is
+  /// either a vector no larger than this size or a scalar.
+  ///
+  /// Instructions with operands or results of different sizes that would be
+  /// split into a different number of fragments are currently left as-is.
+  unsigned ScalarizeMinBits = 0;
+
+  /// Allow the scalarizer pass to scalarize insertelement/extractelement with
+  /// variable index.
+  bool ScalarizeVariableInsertExtract = true;
+
+  /// Allow the scalarizer pass to scalarize loads and store
+  ///
+  /// This is disabled by default because having separate loads and stores makes
+  /// it more likely that the -combiner-alias-analysis limits will be reached.
+  bool ScalarizeLoadStore = false;
 };
 
 class ScalarizerPass : public PassInfoMixin<ScalarizerPass> {

diff --git a/llvm/lib/Passes/PassBuilder.cpp b/llvm/lib/Passes/PassBuilder.cpp
@@ -1047,6 +1047,40 @@ Expected<IPSCCPOptions> parseIPSCCPOptions(StringRef Params) {
   return Result;
 }
 
+Expected<ScalarizerPassOptions> parseScalarizerOptions(StringRef Params) {
+  ScalarizerPassOptions Result;
+  while (!Params.empty()) {
+    StringRef ParamName;
+    std::tie(ParamName, Params) = Params.split(';');
+
+    if (ParamName.consume_front("min-bits=")) {
+      if (ParamName.getAsInteger(0, Result.ScalarizeMinBits)) {
+        return make_error<StringError>(
+            formatv("invalid argument to Scalarizer pass min-bits "
+                    "parameter: '{0}' ",
+                    ParamName)
+                .str(),
+            inconvertibleErrorCode());
+      }
+
+      continue;
+    }
+
+    bool Enable = !ParamName.consume_front("no-");
+    if (ParamName == "load-store")
+      Result.ScalarizeLoadStore = Enable;
+    else if (ParamName == "variable-insert-extract")
+      Result.ScalarizeVariableInsertExtract = Enable;
+    else {
+      return make_error<StringError>(
+          formatv("invalid Scalarizer pass parameter '{0}' ", ParamName).str(),
+          inconvertibleErrorCode());
+    }
+  }
+
+  return Result;
+}
+
 Expected<SROAOptions> parseSROAOptions(StringRef Params) {
   if (Params.empty() || Params == "modify-cfg")
     return SROAOptions::ModifyCFG;

diff --git a/llvm/lib/Passes/PassRegistry.def b/llvm/lib/Passes/PassRegistry.def
@@ -456,7 +456,6 @@ FUNCTION_PASS("reg2mem", RegToMemPass())
 FUNCTION_PASS("safe-stack", SafeStackPass(TM))
 FUNCTION_PASS("sandbox-vectorizer", SandboxVectorizerPass())
 FUNCTION_PASS("scalarize-masked-mem-intrin", ScalarizeMaskedMemIntrinPass())
-FUNCTION_PASS("scalarizer", ScalarizerPass())
 FUNCTION_PASS("sccp", SCCPPass())
 FUNCTION_PASS("select-optimize", SelectOptimizePass(TM))
 FUNCTION_PASS("separate-const-offset-from-gep",
@@ -573,6 +572,12 @@ FUNCTION_PASS_WITH_PARAMS(
       return StackLifetimePrinterPass(dbgs(), Type);
     },
     parseStackLifetimeOptions, "may;must")
+FUNCTION_PASS_WITH_PARAMS(
+    "scalarizer", "ScalarizerPass",
+    [](ScalarizerPassOptions Opts) { return ScalarizerPass(Opts); },
+    parseScalarizerOptions,
+    "load-store;no-load-store;variable-insert-extract;"
+    "no-variable-insert-extract;min-bits=N;")
 FUNCTION_PASS_WITH_PARAMS(
     "separate-const-offset-from-gep", "SeparateConstOffsetFromGEPPass",
     [](bool LowerGEP) { return SeparateConstOffsetFromGEPPass(LowerGEP); },

diff --git a/llvm/lib/Target/DirectX/DirectXPassRegistry.def b/llvm/lib/Target/DirectX/DirectXPassRegistry.def
@@ -23,6 +23,7 @@ MODULE_ANALYSIS("dxil-resource-md", DXILResourceMDAnalysis())
 #ifndef MODULE_PASS
 #define MODULE_PASS(NAME, CREATE_PASS)
 #endif
+MODULE_PASS("dxil-data-scalarization", DXILDataScalarization())
 MODULE_PASS("dxil-intrinsic-expansion", DXILIntrinsicExpansion())
 MODULE_PASS("dxil-op-lower", DXILOpLowering())
 MODULE_PASS("dxil-pretty-printer", DXILPrettyPrinterPass(dbgs()))

diff --git a/llvm/lib/Target/DirectX/DirectXTargetMachine.cpp b/llvm/lib/Target/DirectX/DirectXTargetMachine.cpp
@@ -12,6 +12,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "DirectXTargetMachine.h"
+#include "DXILDataScalarization.h"
 #include "DXILIntrinsicExpansion.h"
 #include "DXILOpLowering.h"
 #include "DXILPrettyPrinter.h"

diff --git a/llvm/lib/Transforms/Scalar/Scalarizer.cpp b/llvm/lib/Transforms/Scalar/Scalarizer.cpp
@@ -39,7 +39,6 @@
 #include "llvm/IR/Value.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Support/Casting.h"
-#include "llvm/Support/CommandLine.h"
 #include "llvm/Transforms/Utils/Local.h"
 #include <cassert>
 #include <cstdint>
@@ -51,28 +50,6 @@ using namespace llvm;
 
 #define DEBUG_TYPE "scalarizer"
 
-static cl::opt<bool> ClScalarizeVariableInsertExtract(
-    "scalarize-variable-insert-extract", cl::init(true), cl::Hidden,
-    cl::desc("Allow the scalarizer pass to scalarize "
-             "insertelement/extractelement with variable index"));
-
-// This is disabled by default because having separate loads and stores
-// makes it more likely that the -combiner-alias-analysis limits will be
-// reached.
-static cl::opt<bool> ClScalarizeLoadStore(
-    "scalarize-load-store", cl::init(false), cl::Hidden,
-    cl::desc("Allow the scalarizer pass to scalarize loads and store"));
-
-// Split vectors larger than this size into fragments, where each fragment is
-// either a vector no larger than this size or a scalar.
-//
-// Instructions with operands or results of different sizes that would be split
-// into a different number of fragments are currently left as-is.
-static cl::opt<unsigned> ClScalarizeMinBits(
-    "scalarize-min-bits", cl::init(0), cl::Hidden,
-    cl::desc("Instruct the scalarizer pass to attempt to keep values of a "
-             "minimum number of bits"));
-
 namespace {
 
 BasicBlock::iterator skipPastPhiNodesAndDbg(BasicBlock::iterator Itr) {
@@ -273,24 +250,14 @@ static Value *concatenate(IRBuilder<> &Builder, ArrayRef<Value *> Fragments,
   return Res;
 }
 
-template <typename T>
-T getWithDefaultOverride(const cl::opt<T> &ClOption,
-                         const std::optional<T> &DefaultOverride) {
-  return ClOption.getNumOccurrences() ? ClOption
-                                      : DefaultOverride.value_or(ClOption);
-}
-
 class ScalarizerVisitor : public InstVisitor<ScalarizerVisitor, bool> {
 public:
   ScalarizerVisitor(DominatorTree *DT, const TargetTransformInfo *TTI,
                     ScalarizerPassOptions Options)
-      : DT(DT), TTI(TTI), ScalarizeVariableInsertExtract(getWithDefaultOverride(
-                              ClScalarizeVariableInsertExtract,
-                              Options.ScalarizeVariableInsertExtract)),
-        ScalarizeLoadStore(getWithDefaultOverride(ClScalarizeLoadStore,
-                                                  Options.ScalarizeLoadStore)),
-        ScalarizeMinBits(getWithDefaultOverride(ClScalarizeMinBits,
-                                                Options.ScalarizeMinBits)) {}
+      : DT(DT), TTI(TTI),
+        ScalarizeVariableInsertExtract(Options.ScalarizeVariableInsertExtract),
+        ScalarizeLoadStore(Options.ScalarizeLoadStore),
+        ScalarizeMinBits(Options.ScalarizeMinBits) {}
 
   bool visit(Function &F);
 

diff --git a/llvm/test/CodeGen/DirectX/rsqrt.ll b/llvm/test/CodeGen/DirectX/rsqrt.ll
@@ -1,5 +1,5 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
-; RUN: opt -S -scalarizer -dxil-op-lower -mtriple=dxil-pc-shadermodel6.3-library %s | FileCheck %s
+; RUN: opt -S -passes='function(scalarizer<load-store>),dxil-op-lower' -mtriple=dxil-pc-shadermodel6.3-library %s | FileCheck %s
 
 ; Make sure dxil operation function calls for rsqrt are generated for float and half.
 

diff --git a/llvm/test/CodeGen/DirectX/scalar-data.ll b/llvm/test/CodeGen/DirectX/scalar-data.ll
@@ -1,4 +1,4 @@
-; RUN: opt -S -dxil-data-scalarization -scalarizer -scalarize-load-store -dxil-op-lower -mtriple=dxil-pc-shadermodel6.3-library %s | FileCheck %s
+; RUN: opt -S -passes='dxil-data-scalarization,function(scalarizer<load-store>),dxil-op-lower' -mtriple=dxil-pc-shadermodel6.3-library %s | FileCheck %s
 ; RUN: llc %s -mtriple=dxil-pc-shadermodel6.3-library --filetype=asm -o - | FileCheck %s
 
 ; Make sure we don't touch arrays without vectors and that can recurse multiple-dimension arrays of vectors

diff --git a/llvm/test/CodeGen/DirectX/scalar-load.ll b/llvm/test/CodeGen/DirectX/scalar-load.ll
@@ -1,4 +1,4 @@
-; RUN: opt -S -dxil-data-scalarization -scalarizer -scalarize-load-store -dxil-op-lower -mtriple=dxil-pc-shadermodel6.3-library %s | FileCheck %s
+; RUN: opt -S -passes='dxil-data-scalarization,function(scalarizer<load-store>),dxil-op-lower' -mtriple=dxil-pc-shadermodel6.3-library %s | FileCheck %s
 ; RUN: llc %s -mtriple=dxil-pc-shadermodel6.3-library --filetype=asm -o - | FileCheck %s
 
 ; Make sure we can load groupshared, static vectors and arrays of vectors

diff --git a/llvm/test/CodeGen/DirectX/scalar-store.ll b/llvm/test/CodeGen/DirectX/scalar-store.ll
@@ -1,4 +1,4 @@
-; RUN: opt -S -dxil-data-scalarization -scalarizer -scalarize-load-store -dxil-op-lower -mtriple=dxil-pc-shadermodel6.3-library %s | FileCheck %s
+; RUN: opt -S -passes='dxil-data-scalarization,scalarizer<load-store>,dxil-op-lower' -mtriple=dxil-pc-shadermodel6.3-library %s | FileCheck %s
 ; RUN: llc %s -mtriple=dxil-pc-shadermodel6.3-library --filetype=asm -o - | FileCheck %s
 
 ; Make sure we can store groupshared, static vectors and arrays of vectors

diff --git a/llvm/test/Transforms/Scalarizer/basic-inseltpoison.ll b/llvm/test/Transforms/Scalarizer/basic-inseltpoison.ll
@@ -1,5 +1,5 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt %s -passes='function(scalarizer,dce)' -scalarize-load-store -S | FileCheck %s
+; RUN: opt %s -passes='function(scalarizer<load-store>,dce)' -S | FileCheck %s
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 
 declare <4 x float> @ext(<4 x float>)

diff --git a/llvm/test/Transforms/Scalarizer/basic.ll b/llvm/test/Transforms/Scalarizer/basic.ll
@@ -1,5 +1,5 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt %s -passes='function(scalarizer,dce)' -scalarize-load-store -S | FileCheck %s
+; RUN: opt %s -passes='function(scalarizer<load-store>,dce)' -S | FileCheck %s
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 
 declare <4 x float> @ext(<4 x float>)

diff --git a/llvm/test/Transforms/Scalarizer/constant-extractelement.ll b/llvm/test/Transforms/Scalarizer/constant-extractelement.ll
@@ -1,5 +1,5 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt %s -passes='function(scalarizer,dce)' -scalarize-load-store -S | FileCheck --check-prefixes=ALL %s
+; RUN: opt %s -passes='function(scalarizer<load-store>,dce)' -S | FileCheck --check-prefixes=ALL %s
 
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 

diff --git a/llvm/test/Transforms/Scalarizer/constant-insertelement.ll b/llvm/test/Transforms/Scalarizer/constant-insertelement.ll
@@ -1,5 +1,5 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt %s -passes='function(scalarizer,dce)' -scalarize-load-store -S | FileCheck --check-prefixes=ALL %s
+; RUN: opt %s -passes='function(scalarizer<load-store>,dce)' -S | FileCheck --check-prefixes=ALL %s
 
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 

diff --git a/llvm/test/Transforms/Scalarizer/dbginfo.ll b/llvm/test/Transforms/Scalarizer/dbginfo.ll
@@ -1,4 +1,4 @@
-; RUN: opt %s -passes='function(scalarizer)' -scalarize-load-store -S --experimental-debuginfo-iterators=false | FileCheck %s
+; RUN: opt %s -passes='function(scalarizer<load-store>)' -S --experimental-debuginfo-iterators=false | FileCheck %s
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 ; FIXME: the test output here changes if we use the RemoveDIs non-intrinsic
 ; debug-info format for the test. Specifically, the intrinsics no longer

diff --git a/llvm/test/Transforms/Scalarizer/min-bits.ll b/llvm/test/Transforms/Scalarizer/min-bits.ll
@@ -1,6 +1,6 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt %s -passes='function(scalarizer,dce)' -scalarize-load-store -scalarize-min-bits=16 -S | FileCheck %s --check-prefixes=CHECK,MIN16
-; RUN: opt %s -passes='function(scalarizer,dce)' -scalarize-load-store -scalarize-min-bits=32 -S | FileCheck %s --check-prefixes=CHECK,MIN32
+; RUN: opt %s -passes='function(scalarizer<load-store;min-bits=16>,dce)' -S | FileCheck %s --check-prefixes=CHECK,MIN16
+; RUN: opt %s -passes='function(scalarizer<load-store;min-bits=32>,dce)' -S | FileCheck %s --check-prefixes=CHECK,MIN32
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 
 define void @load_add_store_v2i16(ptr %pa, ptr %pb) {

diff --git a/llvm/test/Transforms/Scalarizer/opaque-ptr-bug.ll b/llvm/test/Transforms/Scalarizer/opaque-ptr-bug.ll
@@ -1,5 +1,5 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt %s -passes='scalarizer,dce' -S -scalarize-load-store -o - | FileCheck %s
+; RUN: opt %s -passes='scalarizer<load-store>,dce' -S -o - | FileCheck %s
 
 ; This used to crash because the same (pointer) value was scattered by
 ; different amounts.

diff --git a/llvm/test/Transforms/Scalarizer/pass-param-parse-errors.ll b/llvm/test/Transforms/Scalarizer/pass-param-parse-errors.ll
@@ -0,0 +1,9 @@
+; RUN: not opt -passes='scalarizer<unknown>' -disable-output %s 2>&1 | FileCheck -check-prefix=UNKNOWNERR %s
+; RUN: not opt -passes='scalarizer<;>' -disable-output %s 2>&1 | FileCheck -check-prefix=UNKNOWNERR %s
+; RUN: not opt -passes='scalarizer<min-bits=>' -disable-output %s 2>&1 | FileCheck -check-prefix=MINBITS-EMPTY-ERR %s
+; RUN: not opt -passes='scalarizer<min-bits=x>' -disable-output %s 2>&1 | FileCheck -check-prefix=MINBITS-NOTINT-ERR %s
+; RUN: not opt -passes='scalarizer<no-min-bits=10>' -disable-output %s 2>&1 | FileCheck -check-prefix=UNKNOWNERR %s
+
+; UNKNOWNERR: invalid Scalarizer pass parameter '{{.*}}'
+; MINBITS-EMPTY-ERR: invalid argument to Scalarizer pass min-bits parameter: ''
+; MINBITS-NOTINT-ERR: invalid argument to Scalarizer pass min-bits parameter: 'x'
diff --git a/llvm/test/Transforms/Scalarizer/scatter-order.ll b/llvm/test/Transforms/Scalarizer/scatter-order.ll
@@ -1,5 +1,5 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt %s -passes='function(scalarizer)' -scalarize-load-store -S | FileCheck %s
+; RUN: opt %s -passes='function(scalarizer<load-store>)' -S | FileCheck %s
 
 ; This verifies that the order of extract element instructions is
 ; deterministic. In the past we could end up with different results depending

diff --git a/llvm/test/Transforms/Scalarizer/store-bug.ll b/llvm/test/Transforms/Scalarizer/store-bug.ll
@@ -1,4 +1,4 @@
-; RUN: opt -passes='function(scalarizer)' -scalarize-load-store -S < %s | FileCheck %s
+; RUN: opt -passes='function(scalarizer<load-store>)' -S < %s | FileCheck %s
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 
 ; This input caused the scalarizer not to clear cached results

diff --git a/llvm/test/Transforms/Scalarizer/variable-extractelement.ll b/llvm/test/Transforms/Scalarizer/variable-extractelement.ll
@@ -1,7 +1,7 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt %s -passes='function(scalarizer,dce)' -S | FileCheck --check-prefix=DEFAULT %s
-; RUN: opt %s -passes='function(scalarizer,dce)' -scalarize-variable-insert-extract=false -S | FileCheck --check-prefix=OFF %s
-; RUN: opt %s -passes='function(scalarizer,dce)' -scalarize-variable-insert-extract=true -S | FileCheck --check-prefix=DEFAULT %s
+; RUN: opt %s -passes='function(scalarizer<no-variable-insert-extract>,dce)' -S | FileCheck --check-prefix=OFF %s
+; RUN: opt %s -passes='function(scalarizer<variable-insert-extract>,dce)' -S | FileCheck --check-prefix=DEFAULT %s
 
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 

diff --git a/llvm/test/Transforms/Scalarizer/variable-insertelement.ll b/llvm/test/Transforms/Scalarizer/variable-insertelement.ll
@@ -1,7 +1,7 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt %s -passes='function(scalarizer,dce)' -S | FileCheck --check-prefix=DEFAULT %s
-; RUN: opt %s -passes='function(scalarizer,dce)' -scalarize-variable-insert-extract=false -S | FileCheck --check-prefix=OFF %s
-; RUN: opt %s -passes='function(scalarizer,dce)' -scalarize-variable-insert-extract=true -S | FileCheck --check-prefix=DEFAULT %s
+; RUN: opt %s -passes='function(scalarizer<no-variable-insert-extract>,dce)' -S | FileCheck --check-prefix=OFF %s
+; RUN: opt %s -passes='function(scalarizer<variable-insert-extract>,dce)' -S | FileCheck --check-prefix=DEFAULT %s
 
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 

diff --git a/llvm/test/Transforms/Scalarizer/vector-of-pointer-to-vector.ll b/llvm/test/Transforms/Scalarizer/vector-of-pointer-to-vector.ll
@@ -1,5 +1,5 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt %s -passes='function(scalarizer,dce)' -scalarize-load-store -S | FileCheck %s
+; RUN: opt %s -passes='function(scalarizer<load-store>,dce)' -S | FileCheck %s
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 
 define <1 x i32> @f1(<1 x ptr> %src, i32 %index) {