Correct codegenoption assigning from semantic defines (#3061)

For structurize-returns, the codegenopts assigned were a copy of those
the compiler possesses. This is by design in clang as explicitly stated
in comments. As a result, when the compiler codegenopts are updated, the
codegenmodule's are not.

By adding the ability to query the optimization enables from the hlsl
extension helper, these options can be reflected.

Additionally, the structurize-returns flag would have required a define
with a '-' in it, which isn't possible. So now it converts between '_'
for defines and '-' for options, which is consistent with how other
flags are interpretted.

Adds a unit test similar to that of disable gvn that makes a simple
check to verify that the resturn structurization is enabled.

include/dxc/HLSL/HLSLExtensionsCodegenHelper.h

@@ -63,6 +63,10 @@ public:
   // Write semantic defines as metadata in the module.
   virtual SemanticDefineErrorList WriteSemanticDefines(llvm::Module *M) = 0;
 
+  // Query the named option enable
+  // Needed because semantic defines may have set it since options were copied
+  virtual bool IsOptionEnabled(std::string option) = 0;
+
   // Get the name to use for the dxil intrinsic function.
   virtual std::string GetIntrinsicName(unsigned opcode) = 0;
 

tools/clang/lib/CodeGen/CGHLSLMS.cpp

@@ -3307,9 +3307,7 @@ void CGMSHLSLRuntime::FinishCodeGen() {
     ExtensionCodeGen(HLM, CGM);
   }
 
-  if (CGM.getCodeGenOpts().HLSLOptimizationToggles.count("structurize-returns") &&
-      CGM.getCodeGenOpts().HLSLOptimizationToggles.find("structurize-returns")->second)
-    StructurizeMultiRet(M, m_ScopeMap, bWaveEnabledStage, m_DxBreaks);
+  StructurizeMultiRet(M, CGM, m_ScopeMap, bWaveEnabledStage, m_DxBreaks);
 
   FinishEntries(HLM, Entry, CGM, entryFunctionMap, HSEntryPatchConstantFuncAttr,
                 patchConstantFunctionMap, patchConstantFunctionPropsMap);

tools/clang/lib/CodeGen/CGHLSLMSFinishCodeGen.cpp

@@ -3015,9 +3015,19 @@ void StructurizeMultiRetFunction(Function *F, ScopeInfo &ScopeInfo,
 } // namespace
 
 namespace CGHLSLMSHelper {
-void StructurizeMultiRet(Module &M, DenseMap<Function *, ScopeInfo> &ScopeMap,
+void StructurizeMultiRet(Module &M, clang::CodeGen::CodeGenModule &CGM,
+                         DenseMap<Function *, ScopeInfo> &ScopeMap,
                          bool bWaveEnabledStage,
                          SmallVector<BranchInst *, 16> &DxBreaks) {
+  if (CGM.getCodeGenOpts().HLSLExtensionsCodegen) {
+    if (!CGM.getCodeGenOpts().HLSLExtensionsCodegen->IsOptionEnabled("structurize-returns"))
+      return;
+  } else {
+    if (!CGM.getCodeGenOpts().HLSLOptimizationToggles.count("structurize-returns") ||
+        !CGM.getCodeGenOpts().HLSLOptimizationToggles.find("structurize-returns")->second)
+      return;
+  }
+
   for (Function &F : M) {
     if (F.isDeclaration())
       continue;
@@ -3027,4 +3037,4 @@ void StructurizeMultiRet(Module &M, DenseMap<Function *, ScopeInfo> &ScopeMap,
     StructurizeMultiRetFunction(&F, it->second, bWaveEnabledStage, DxBreaks);
   }
 }
-} // namespace CGHLSLMSHelper
+} // namespace CGHLSLMSHelper

tools/clang/lib/CodeGen/CGHLSLMSHelper.h

@@ -184,6 +184,7 @@ void UpdateLinkage(
     llvm::StringMap<PatchConstantInfo> &patchConstantFunctionMap);
 
 void StructurizeMultiRet(llvm::Module &M,
+                         clang::CodeGen::CodeGenModule &CGM,
                          llvm::DenseMap<llvm::Function *, ScopeInfo> &ScopeMap,
                          bool bWaveEnabledStage,
                          llvm::SmallVector<llvm::BranchInst *, 16> &DxBreaks);

tools/clang/tools/dxcompiler/dxcompilerobj.cpp

@@ -300,12 +300,20 @@ private:
       }
 
       // Add semantic defines to option flag equivalents
-      if (!define.Name.compare(prefixPos, enableStr.length(), enableStr))
-        optToggles[StringRef(define.Name.substr(prefixPos + enableStr.length())).lower()] = true;
-      else if (!define.Name.compare(prefixPos, disableStr.length(), disableStr))
-        optToggles[StringRef(define.Name.substr(prefixPos + disableStr.length())).lower()] = false;
-      else if (!define.Name.compare(prefixPos, selectStr.length(), selectStr))
-        optSelects[StringRef(define.Name.substr(prefixPos + selectStr.length())).lower()] = define.Value;
+      // Convert define-style '_' into option-style '-' and lowercase everything
+      if (!define.Name.compare(prefixPos, enableStr.length(), enableStr)) {
+        std::string optName = define.Name.substr(prefixPos + enableStr.length());
+        std::replace(optName.begin(), optName.end(), '_', '-');
+        optToggles[StringRef(optName).lower()] = true;
+      } else if (!define.Name.compare(prefixPos, disableStr.length(), disableStr)) {
+        std::string optName = define.Name.substr(prefixPos + disableStr.length());
+        std::replace(optName.begin(), optName.end(), '_', '-');
+        optToggles[StringRef(optName).lower()] = false;
+      } else if (!define.Name.compare(prefixPos, selectStr.length(), selectStr)) {
+        std::string optName = define.Name.substr(prefixPos + selectStr.length());
+        std::replace(optName.begin(), optName.end(), '_', '-');
+        optSelects[StringRef(optName).lower()] = define.Value;
+      }
     }
 
     // Add root node with pointers to all define metadata nodes.
@@ -351,6 +359,11 @@ public:
     return errors;
   }
 
+  virtual bool IsOptionEnabled(std::string option) override {
+    return m_CI.getCodeGenOpts().HLSLOptimizationToggles.count(option) &&
+      m_CI.getCodeGenOpts().HLSLOptimizationToggles.find(option)->second;
+  }
+
   virtual std::string GetIntrinsicName(UINT opcode) override {
     return m_langExtensionsHelper.GetIntrinsicName(opcode);
   }

tools/clang/unittests/HLSL/ExtensionTest.cpp

@@ -454,7 +454,8 @@ public:
   TEST_METHOD(DefineNoValidatorOk)
   TEST_METHOD(DefineFromMacro)
   TEST_METHOD(DefineContradictionFail)
-  TEST_METHOD(OptionFromDefine)
+  TEST_METHOD(OptionFromDefineGVN)
+  TEST_METHOD(OptionFromDefineStructurizeReturns)
   TEST_METHOD(TargetTriple)
   TEST_METHOD(IntrinsicWhenAvailableThenUsed)
   TEST_METHOD(CustomIntrinsicName)
@@ -652,7 +653,7 @@ TEST_F(ExtensionTest, DefineContradictionFail) {
 }
 
 // Test setting of codegen options from semantic defines
-TEST_F(ExtensionTest, OptionFromDefine) {
+TEST_F(ExtensionTest, OptionFromDefineGVN) {
 
   Compiler c(m_dllSupport);
   c.RegisterSemanticDefine(L"FOO*");
@@ -674,6 +675,36 @@ TEST_F(ExtensionTest, OptionFromDefine) {
   VERIFY_IS_TRUE(regex.match(disassembly));
 }
 
+// Test setting of codegen options from semantic defines
+TEST_F(ExtensionTest, OptionFromDefineStructurizeReturns) {
+
+  Compiler c(m_dllSupport);
+  c.RegisterSemanticDefine(L"FOO*");
+  c.Compile(
+    "int i;\n"
+    "float main(float4 a:A) : SV_Target {\n"
+    "float c = 0;\n"
+    "if (i < 0) {\n"
+    "  if (a.w > 2)\n"
+    "    return -1;\n"
+    "  c += a.z;\n"
+    "}\n"
+    "return c;\n"
+    "}\n",
+    { L"/Vd", L"-fcgl", L"-DFOO_ENABLE_STRUCTURIZE_RETURNS" },
+    {}
+  );
+
+  std::string disassembly = c.Disassemble();
+  // Verify that structurize returns is enabled by the presence
+  // of the associated annotation. Just a simple test to
+  // verify that it's on. No need to go into detail here
+  llvm::Regex regex("bReturned.* = alloca i1");
+  std::string regexErrors;
+  VERIFY_IS_TRUE(regex.isValid(regexErrors));
+  VERIFY_IS_TRUE(regex.match(disassembly));
+}
+
 
 TEST_F(ExtensionTest, TargetTriple) {
   Compiler c(m_dllSupport);