Disable non-legacy cbuffer flags (microsoft#6559)

Greg Roth · web-flow · commit 1c66c4d798b0 · 2024-04-23T11:59:49.000-07:00
These flags had some effect on load code generation, but never worked right and were not tested in the DXC compiler let alone with drivers. This removes any notion of the flags beyond option processing and produces a warning message that the flag is not supported and will be ignored. The flags are both not listed in help anymore either. A simple OptionsTest and lit test verify that the warning message is produced for either flag and existing tests that used the flag are updated or removed. Incidentally fixed the capitalization of an existing options warning message. Fixes microsoft#6306
diff --git a/include/dxc/HLSL/HLModule.h b/include/dxc/HLSL/HLModule.h
@@ -51,17 +51,16 @@ class OP;
 struct HLOptions {
   HLOptions()
       : bDefaultRowMajor(false), bIEEEStrict(false), bAllResourcesBound(false),
-        bDisableOptimizations(false), bLegacyCBufferLoad(false),
-        PackingStrategy(0), bUseMinPrecision(false), bDX9CompatMode(false),
-        bFXCCompatMode(false), bLegacyResourceReservation(false),
-        bForceZeroStoreLifetimes(false), unused(0) {}
+        bDisableOptimizations(false), PackingStrategy(0),
+        bUseMinPrecision(false), bDX9CompatMode(false), bFXCCompatMode(false),
+        bLegacyResourceReservation(false), bForceZeroStoreLifetimes(false),
+        unused(0) {}
   uint32_t GetHLOptionsRaw() const;
   void SetHLOptionsRaw(uint32_t data);
   unsigned bDefaultRowMajor : 1;
   unsigned bIEEEStrict : 1;
   unsigned bAllResourcesBound : 1;
   unsigned bDisableOptimizations : 1;
-  unsigned bLegacyCBufferLoad : 1;
   unsigned PackingStrategy : 2;
   static_assert((unsigned)DXIL::PackingStrategy::Invalid < 4,
                 "otherwise 2 bits is not enough to store PackingStrategy");
diff --git a/include/dxc/Support/HLSLOptions.h b/include/dxc/Support/HLSLOptions.h
@@ -198,7 +198,6 @@ class DxcOpts {
   bool UseHexLiterals = false;            // OPT_Lx
   bool UseInstructionByteOffsets = false; // OPT_No
   bool UseInstructionNumbers = false;     // OPT_Ni
-  bool NotUseLegacyCBufLoad = false;      // OPT_no_legacy_cbuf_layout
   bool PackPrefixStable = false;          // OPT_pack_prefix_stable
   bool PackOptimized = false;             // OPT_pack_optimized
   bool DisplayIncludeProcess = false;     // OPT__vi
diff --git a/include/dxc/Support/HLSLOptions.td b/include/dxc/Support/HLSLOptions.td
@@ -278,7 +278,7 @@ def flegacy_macro_expansion : Flag<["-", "/"], "flegacy-macro-expansion">, Group
     HelpText<"Expand the operands before performing token-pasting operation (fxc behavior)">;
 def flegacy_resource_reservation : Flag<["-", "/"], "flegacy-resource-reservation">, Group<hlslcomp_Group>, Flags<[CoreOption, DriverOption]>,
     HelpText<"Reserve unused explicit register assignments for compatibility with shader model 5.0 and below">;
-def no_legacy_cbuf_layout : Flag<["-", "/"], "no-legacy-cbuf-layout">, Group<hlslcomp_Group>, Flags<[CoreOption]>,
+def no_legacy_cbuf_layout : Flag<["-", "/"], "no-legacy-cbuf-layout">, Group<hlslcomp_Group>, Flags<[CoreOption, HelpHidden]>,
   HelpText<"Do not use legacy cbuffer load">;
 def not_use_legacy_cbuf_load_ : Flag<["-", "/"], "not_use_legacy_cbuf_load">, Group<hlslcomp_Group>, Flags<[CoreOption, HelpHidden]>,
   HelpText<"Do not use legacy cbuffer load">;
diff --git a/lib/DxcSupport/HLSLOptions.cpp b/lib/DxcSupport/HLSLOptions.cpp
@@ -570,7 +570,7 @@ int ReadDxcOpts(const OptTable *optionTable, unsigned flagsToInclude,
               // Args.getLastArgValue(OPT_INPUT) get expect Input.
               InputArg->getValues()[0] = PrevInputArg->getValues()[0];
             }
-            errors << "Warning: -P " << opts.Preprocess
+            errors << "warning: -P " << opts.Preprocess
                    << " is deprecated, please use -P -Fi " << opts.Preprocess
                    << " instead.\n";
             break;
@@ -762,10 +762,14 @@ int ReadDxcOpts(const OptTable *optionTable, unsigned flagsToInclude,
   opts.DefaultColMajor = Args.hasFlag(OPT_Zpc, OPT_INVALID, false);
   opts.DumpBin = Args.hasFlag(OPT_dumpbin, OPT_INVALID, false);
   opts.Link = Args.hasFlag(OPT_link, OPT_INVALID, false);
-  opts.NotUseLegacyCBufLoad =
+  bool NotUseLegacyCBufLoad =
       Args.hasFlag(OPT_no_legacy_cbuf_layout, OPT_INVALID, false);
-  opts.NotUseLegacyCBufLoad = Args.hasFlag(
-      OPT_not_use_legacy_cbuf_load_, OPT_INVALID, opts.NotUseLegacyCBufLoad);
+  NotUseLegacyCBufLoad = Args.hasFlag(OPT_not_use_legacy_cbuf_load_,
+                                      OPT_INVALID, NotUseLegacyCBufLoad);
+  if (NotUseLegacyCBufLoad)
+    errors << "warning: -no-legacy-cbuf-layout"
+           << " is no longer supported and will be ignored."
+           << " Future releases will not recognize it.\n";
   opts.PackPrefixStable =
       Args.hasFlag(OPT_pack_prefix_stable, OPT_INVALID, false);
   opts.PackPrefixStable =
diff --git a/lib/HLSL/HLOperationLower.cpp b/lib/HLSL/HLOperationLower.cpp
@@ -51,7 +51,6 @@ struct HLOperationLowerHelper {
   Type *i8Ty;
   DxilTypeSystem &dxilTypeSys;
   DxilFunctionProps *functionProps;
-  bool bLegacyCBufferLoad;
   DataLayout dataLayout;
   SmallDenseMap<Type *, Type *, 4> loweredTypes;
   typedef std::pair<DxilWaveMatrixProperties, Constant *> WaveMatrix_Props;
@@ -77,7 +76,6 @@ HLOperationLowerHelper::HLOperationLowerHelper(HLModule &HLM)
   functionProps = nullptr;
   if (HLM.HasDxilFunctionProps(EntryFunc))
     functionProps = &HLM.GetDxilFunctionProps(EntryFunc);
-  bLegacyCBufferLoad = HLM.GetHLOptions().bLegacyCBufferLoad;
 }
 
 const HLOperationLowerHelper::WaveMatrix_Props &
@@ -7459,20 +7457,6 @@ void TranslateCBGep(GetElementPtrInst *GEP, Value *handle, Value *baseOffset,
   }
 }
 
-void TranslateCBOperations(Value *handle, Value *ptr, Value *offset, OP *hlslOP,
-                           DxilTypeSystem &dxilTypeSys, const DataLayout &DL,
-                           HLObjectOperationLowerHelper *pObjHelper) {
-  auto User = ptr->user_begin();
-  auto UserE = ptr->user_end();
-  for (; User != UserE;) {
-    // Must be Instruction.
-    Instruction *I = cast<Instruction>(*(User++));
-    TranslateCBAddressUser(I, handle, offset, hlslOP,
-                           /*prevFieldAnnotation*/ nullptr, dxilTypeSys, DL,
-                           pObjHelper);
-  }
-}
-
 Value *GenerateCBLoadLegacy(Value *handle, Value *legacyIdx,
                             unsigned channelOffset, Type *EltTy, OP *hlslOP,
                             IRBuilder<> &Builder) {
@@ -9057,14 +9041,8 @@ void TranslateHLSubscript(CallInst *CI, HLSubscriptOpcode opcode,
     dxilutil::MergeGepUse(CI);
     // Resource ptr.
     Value *handle = CI->getArgOperand(HLOperandIndex::kSubscriptObjectOpIdx);
-    if (helper.bLegacyCBufferLoad)
-      TranslateCBOperationsLegacy(handle, CI, hlslOP, helper.dxilTypeSys,
-                                  helper.dataLayout, pObjHelper);
-    else {
-      TranslateCBOperations(handle, CI, /*offset*/ hlslOP->GetU32Const(0),
-                            hlslOP, helper.dxilTypeSys,
-                            CI->getModule()->getDataLayout(), pObjHelper);
-    }
+    TranslateCBOperationsLegacy(handle, CI, hlslOP, helper.dxilTypeSys,
+                                helper.dataLayout, pObjHelper);
     Translated = true;
     return;
   } else if (opcode == HLSubscriptOpcode::DoubleSubscript) {
diff --git a/tools/clang/include/clang/Frontend/CodeGenOptions.h b/tools/clang/include/clang/Frontend/CodeGenOptions.h
@@ -185,8 +185,6 @@ class CodeGenOptions : public CodeGenOptionsBase {
   bool HLSLAllowPreserveValues = false;
   /// Whether we fail compilation if loop fails to unroll
   bool HLSLOnlyWarnOnUnrollFail = false;
-  /// Whether use legacy cbuffer load.
-  bool HLSLNotUseLegacyCBufLoad = false;
   /// Whether use legacy resource reservation.
   bool HLSLLegacyResourceReservation = false;
   /// Set [branch] on every if.
diff --git a/tools/clang/lib/CodeGen/CGHLSLMS.cpp b/tools/clang/lib/CodeGen/CGHLSLMS.cpp
@@ -394,7 +394,6 @@ CGMSHLSLRuntime::CGMSHLSLRuntime(CodeGenModule &CGM)
   HLOptions opts;
   opts.bIEEEStrict = CGM.getCodeGenOpts().UnsafeFPMath;
   opts.bDisableOptimizations = CGM.getCodeGenOpts().DisableLLVMOpts;
-  opts.bLegacyCBufferLoad = !CGM.getCodeGenOpts().HLSLNotUseLegacyCBufLoad;
   opts.bAllResourcesBound = CGM.getCodeGenOpts().HLSLAllResourcesBound;
   opts.bResMayAlias = CGM.getCodeGenOpts().HLSLResMayAlias;
   opts.PackingStrategy = CGM.getCodeGenOpts().HLSLSignaturePackingStrategy;
diff --git a/tools/clang/test/DXC/no_legacy_cbuf_layout_warning.hlsl b/tools/clang/test/DXC/no_legacy_cbuf_layout_warning.hlsl
@@ -0,0 +1,6 @@
+// RUN: %dxc -T vs_6_0 %s -no-legacy-cbuf-layout 2>&1 | FileCheck %s
+// RUN: %dxc -T vs_6_0 %s -not_use_legacy_cbuf_load 2>&1 | FileCheck %s
+
+// CHECK: warning: -no-legacy-cbuf-layout is no longer supported and will be ignored. Future releases will not recognize it.
+
+void main() {}
diff --git a/tools/clang/test/HLSLFileCheck/hlsl/objects/Cbuffer/cbufferAlloc.hlsl b/tools/clang/test/HLSLFileCheck/hlsl/objects/Cbuffer/cbufferAlloc.hlsl
diff --git a/tools/clang/test/HLSLFileCheck/hlsl/objects/Cbuffer/resource-in-cb.hlsl b/tools/clang/test/HLSLFileCheck/hlsl/objects/Cbuffer/resource-in-cb.hlsl
@@ -1,4 +1,4 @@
-// RUN: %dxc -E main -T ps_6_0 -not_use_legacy_cbuf_load %s  | FileCheck %s
+// RUN: %dxc -E main -T ps_6_0 %s  | FileCheck %s
 
 // CHECK: Tex1
 
diff --git a/tools/clang/test/HLSLFileCheck/hlsl/objects/Cbuffer/uint64_1.hlsl b/tools/clang/test/HLSLFileCheck/hlsl/objects/Cbuffer/uint64_1.hlsl
@@ -1,4 +1,4 @@
-// RUN: %dxc -E main -T ps_6_0 -not_use_legacy_cbuf_load  %s | FileCheck %s
+// RUN: %dxc -E main -T ps_6_0 %s | FileCheck %s
 
 // CHECK: 64-Bit integer
 // CHECK: add i64
diff --git a/tools/clang/test/HLSLFileCheck/hlsl/objects/Cbuffer/uint64_2.hlsl b/tools/clang/test/HLSLFileCheck/hlsl/objects/Cbuffer/uint64_2.hlsl
@@ -1,4 +1,4 @@
-// RUN: %dxilver 1.1 | %dxc -E main -T cs_6_0 -validator-version 1.1 -not_use_legacy_cbuf_load  %s | FileCheck %s
+// RUN: %dxilver 1.1 | %dxc -E main -T cs_6_0 -validator-version 1.1 %s | FileCheck %s
 
 // CHECK: 64-Bit integer
 // CHECK: dx.op.bufferStore.i32
diff --git a/tools/clang/test/HLSLFileCheck/hlsl/operators/binary/shift.hlsl b/tools/clang/test/HLSLFileCheck/hlsl/operators/binary/shift.hlsl
@@ -1,4 +1,4 @@
-// RUN: %dxc -E main -T ps_6_0  -not_use_legacy_cbuf_load %s | FileCheck %s
+// RUN: %dxc -E main -T ps_6_0 %s | FileCheck %s
 
 // The shift for hlsl only use the LSB 5 bits (0-31 range) of src1 for int/uint.
 // CHECK: shl i32 {{.*}}, 18
diff --git a/tools/clang/tools/dxcompiler/dxcompilerobj.cpp b/tools/clang/tools/dxcompiler/dxcompilerobj.cpp
@@ -1505,8 +1505,6 @@ class DxcCompiler : public IDxcCompiler3,
     compiler.getCodeGenOpts().HLSLOverrideSemDefs = Opts.OverrideSemDefs;
     compiler.getCodeGenOpts().HLSLPreferControlFlow = Opts.PreferFlowControl;
     compiler.getCodeGenOpts().HLSLAvoidControlFlow = Opts.AvoidFlowControl;
-    compiler.getCodeGenOpts().HLSLNotUseLegacyCBufLoad =
-        Opts.NotUseLegacyCBufLoad;
     compiler.getCodeGenOpts().HLSLLegacyResourceReservation =
         Opts.LegacyResourceReservation;
     compiler.getCodeGenOpts().HLSLDefines = defines;
diff --git a/tools/clang/unittests/HLSL/OptionsTest.cpp b/tools/clang/unittests/HLSL/OptionsTest.cpp
@@ -78,6 +78,7 @@ class OptionsTest : public ::testing::Test {
   // TEST_METHOD(CopyOptionsWhenMultipleThenOK)
 
   TEST_METHOD(ReadOptionsJoinedWithSpacesThenOK)
+  TEST_METHOD(ReadOptionsNoNonLegacyCBuffer)
 
   TEST_METHOD(TestPreprocessOption)
 
@@ -96,16 +97,24 @@ class OptionsTest : public ::testing::Test {
     EXPECT_EQ(shouldMessage, !errorStream.str().empty());
     return opts;
   }
+
+  // Test variant that verifies expected pass condition
+  // and checks error output to match given message.
   void ReadOptsTest(const MainArgs &mainArgs, unsigned flagsToInclude,
-                    const char *expectErrorMsg) {
+                    bool shouldFail, const char *expectErrorMsg) {
     std::string errorString;
     llvm::raw_string_ostream errorStream(errorString);
     std::unique_ptr<DxcOpts> opts = llvm::make_unique<DxcOpts>();
     int result = ReadDxcOpts(getHlslOptTable(), flagsToInclude, mainArgs,
                              *(opts.get()), errorStream);
-    EXPECT_EQ(result, 1);
+    EXPECT_EQ(shouldFail, result != 0);
     VERIFY_ARE_EQUAL_STR(expectErrorMsg, errorStream.str().c_str());
   }
+
+  void ReadOptsTest(const MainArgs &mainArgs, unsigned flagsToInclude,
+                    const char *expectErrorMsg) {
+    ReadOptsTest(mainArgs, flagsToInclude, true /*shouldFail*/, expectErrorMsg);
+  }
 };
 
 TEST_F(OptionsTest, ReadOptionsWhenExtensionsThenOK) {
@@ -369,6 +378,26 @@ TEST_F(OptionsTest, ReadOptionsJoinedWithSpacesThenOK) {
   }
 }
 
+TEST_F(OptionsTest, ReadOptionsNoNonLegacyCBuffer) {
+  {
+    const wchar_t *Args[] = {L"exe.exe", L"/T  ", L"ps_6_0", L"hlsl.hlsl",
+                             L"-no-legacy-cbuf-layout"};
+    MainArgsArr ArgsArr(Args);
+    ReadOptsTest(ArgsArr, DxcFlags, false /*shouldFail*/,
+                 "warning: -no-legacy-cbuf-layout is no longer supported and "
+                 "will be ignored. Future releases will not recognize it.\n");
+  }
+
+  {
+    const wchar_t *Args[] = {L"exe.exe", L"/T  ", L"ps_6_0", L"hlsl.hlsl",
+                             L"-not_use_legacy_cbuf_load"};
+    MainArgsArr ArgsArr(Args);
+    ReadOptsTest(ArgsArr, DxcFlags, false /*shouldFail*/,
+                 "warning: -no-legacy-cbuf-layout is no longer supported and "
+                 "will be ignored. Future releases will not recognize it.\n");
+  }
+}
+
 static void VerifyPreprocessOption(llvm::StringRef command,
                                    const char *ExpectOutput,
                                    const char *ErrMsg) {
@@ -393,7 +422,7 @@ TEST_F(OptionsTest, TestPreprocessOption) {
   VerifyPreprocessOption("/T ps_6_0 -Fi out.pp -P input.hlsl", "out.pp", "");
   VerifyPreprocessOption("/T ps_6_0 -P -Fi out.pp input.hlsl", "out.pp", "");
   const char *Warning =
-      "Warning: -P out.pp is deprecated, please use -P -Fi out.pp instead.\n";
+      "warning: -P out.pp is deprecated, please use -P -Fi out.pp instead.\n";
   VerifyPreprocessOption("/T ps_6_0 -P out.pp input.hlsl", "out.pp", Warning);
   VerifyPreprocessOption("/T ps_6_0 input.hlsl -P out.pp ", "out.pp", Warning);
 }

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-// RUN: %dxc -E main -T ps_6_0 -not_use_legacy_cbuf_load %s \| FileCheck %s`
	`1`	`+// RUN: %dxc -E main -T ps_6_0 %s \| FileCheck %s`
`2`	`2`
`3`	`3`	`// CHECK: Tex1`
`4`	`4`