[PATCH] D42762: Rewrite the VS Integration Scripts

Mon Feb 5 14:27:32 PST 2018

rnk 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:
> > > > > > 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.
> Sure, it's not terribly bad.  But I'd like to do something better than just "not terribly bad".  :)
> 
> There are other issues with auto-generating the file from the td as well.  When an option takes multiple values (such as `/arch:`), the specific values it takes are not always present in the TD file.  You mentioned `/arch:AVX512` earlier, but that value is not in the td file anywhere.
> 
> That doesn't mean it's impossible, we can probably address all of the various issues to get the td file to a point where this file could be auto-generated.  But I don't think that work should hold this up, as I still maintain that the solution here is strictly better from the user's perspective than the old method of doing nothing.
I don't think we'd get much value from table generating them. This file contains a lot of information and organization that we'd have to encode in tablegen. Every option would need its property sheet category, name, and display name, none of which we currently encode.

If we did want to automatically fill in undescribed options somehow, I'd suggest storing the property sheet information separately kind of like we currently do for the attribute documentation, and then "joining" the two files in tablegen to make this XML file. I just don't think it's worth it, though.

https://reviews.llvm.org/D42762