[PATCH] D42762: Rewrite the VS Integration Scripts

[Hans Wennborg via Phabricator via llvm-commits]

hans added inline comments.


================
Comment at: llvm/tools/msbuild/clang-cl.xml:1
+<?xml version="1.0" encoding="utf-8"?>
+<Rule Name="clang-cl" PageTemplate="tool" DisplayName="C/C++" SwitchPrefix="/" Order="10" xmlns="http://schemas.microsoft.com/build/2009/properties" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:sys="clr-namespace:System;assembly=mscorlib">
----------------
zturner wrote:
> hans wrote:
> > zturner wrote:
> > > hans wrote:
> > > > zturner wrote:
> > > > > hans wrote:
> > > > > > I thought you were going to leave the flag logic out of this patch?
> > > > > I thought Nico said to only separate out the option remapping into another patch.  I took all of that out.
> > > > This file still attempts to duplicate the logic of what flags clang-cl supports, which are ignored, etc. I'd prefer if we didn't do that.
> > > > 
> > > > To repeat my concerns:
> > > > 
> > > > 1. This list of flags would need to be maintained, and I expect it will always end up being incomplete or inaccurate in some other way. The only way to keep it accurate would be if it's auto-generated from clang-cl's tablegen file. But that ties it to a specific clang-cl version, which is the next problem
> > > > 
> > > > 2. It ties it to a particular clang-cl version, which goes against the point of decoupling the VS integration from the toolchain. As a concrete example, your file is missing /arch:AVX512 and /arch:AVX512F, so those should be added, but only when the user is using a Clang >= r323433. Of course the user could them manually, but why should some flags be listed and some not? And maybe we could handle this with version logic, but now we're getting into *really* hairy maintenance.
> > > I can move this to another patch if you want, but I have to say that presenting a clang-cl tailored UI of the options is something that I feel very strongly about.  In fact, the main reason I volunteered to take on this effort is specifically because I wanted to make sure we do this.
> > > 
> > > We can get into how it would have to be maintained etc, but all of that is irrelevant IMO because I'm approaching this from the perspective of the user.  As someone who has used VS pretty heavily for close to 20 years, this is absolutely a large value-add for the user.
> > > 
> > > To re-iterate though, I don't see where the perceived maintenance comes from.  You're already suggesting that I do nothing, which means that the user will already have to manually add options anyway if the version of VS doesn't match up with the version of clang-cl exactly.  So what I'm doing here is imperfect (as is the alternative of doing nothing), but is //strictly better//.  I don't see how that could be perceived as anything other than a win.
> > > 
> > > Version logic isn't even that hairy of maintenance.  Every time we ship a new LLVM release (which seems to be about once a year), we copy the file and make the 1 or 2 necessary modifications to the copied file.
> > I think a clang-cl tailored UI would be really nice, but I don't think it's sustainable to maintain this file by hand. If we want to expose clang-cl's options this way, I think we need to auto-generate it from the .td file, and that it needs to be tied to the clang-cl version whose options are being exposed.
> > 
> > We ship a major release twice a year, typically two more stable (x.0.1) releases per year, and in addition to that I ship snapshots about once or twice a month. If we're going to present clang-cl's options here, I think the file needs to stay in sync with clang-cl's options.
> Having it be auto-generated from the .td file is a good idea, but it's quite a bit of extra work and it may not be possible to do it in a useful way (for example, MSVC's UI displays the options in categories, which are defined just below this comment in Phab.  Having the auto-generated options be correctly categorized seems difficult / impossible).
> 
> It's worth trying, and if we can do it then I agree it would be worth doing, but in the meantime I think the approach here is still strictly better than doing nothing, even if this file were never touched again (which is unlikely).
We can add categories to the flags in the .td file.

The "doing nothing" that we currently do is expose Visual Studio's choice of flags and then let clang-cl handle them. I don't think that's terribly bad.


https://reviews.llvm.org/D42762