[PATCH] D42762: Rewrite the VS Integration Scripts

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: llvm/tools/msbuild/LLVM.Cpp.Common.props:42
+    <LLVMInstallDir Condition="'$(LLVMInstallDir)' == ''">$(Registry:HKEY_LOCAL_MACHINE\SOFTWARE\Wow6432Node\LLVM at LLVM)</LLVMInstallDir>
+    <ClangClExecutable>$(LLVMInstallDir)bin\clang-cl.exe</ClangClExecutable>
+  </PropertyGroup>
----------------
smeenai wrote:
> Is there a way to add some sort of error here if LLVMInstallDir is still empty? Is it worth it?
I'm not sure it's worth it.  We could maybe warn when the project is loaded if the path is invalid, but I don't think we should do anything special for `$(LLVMInstallDir)` specifically, because ultimately the user can just change this to something else, so having a valid install dir is not even necessary.  So for a user that never wants to install LLVM and just set the path to clang, they might get annoyed by constantly seeing a warning which they don't care about.  If they do try to use it, they'll get an error pretty quickly along the lines of `Unable to find path file \bin\clang-cl.exe` so hopefully that's a sufficient clue?  I could probably try to catch this in `BeforeClCompile` and print a better error message if you think the standard error message isn't clear enough.


================
Comment at: llvm/tools/msbuild/LLVM.Cpp.Common.props:72
+
+      <AdditionalOptions>-m$(PlatformArchitecture) %(AdditionalOptions)</AdditionalOptions>
+    </ClCompile>
----------------
smeenai wrote:
> Not that it really matters right now, but this wouldn't do the right thing for arm or arm64, right?
Right, we'd need something else here for arm.


================
Comment at: llvm/tools/msbuild/LLVM.Cpp.Common.targets:56
+    <!-- Warn if Whole Program Optimization is enabled, and then ignore it. -->
+    <Warning Condition="'%(ClCompile.WholeProgramOptimization)' == 'true'"
+             File="@(ClCompile)(0,0)"
----------------
smeenai wrote:
> If users want to use LTO with clang-cl + lld-link, are they just expected to set the appropriate compiler and linker options manually?
For now yes.  Even though they are conceptually the same thing, they are incompatible with each other from an implementation standpoint.  So, for example, you couldn't use MSVC WPO object files with lld-link, for example, or vice versa.  This VS integration right now, for example, doesn't even add support for using lld-link (only clang-cl), so use of this option right now would break the build, as link.exe would be trying to link llvm bitcode files.  So I think it makes sense to (eventually) provide a separate UI option for LLVM LTO, perhaps under the new LLVM UI page.


https://reviews.llvm.org/D42762