[PATCH] D42762: Rewrite the VS Integration Scripts

Wed Feb 7 01:46:02 PST 2018

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:
> rnk wrote:
> > hans wrote:
> > > rnk wrote:
> > > > rnk wrote:
> > > > > 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.
> > > > I think part of the reason why I don't like the idea of loading this all into tablegen is that editing tablegen files sucks. We have all these ad-hoc conventions for adding line breaks, but there's no editor support for them or anything. I think if they were YAML files, it would be a lot easier to add new fields to options to help us auto-generate property sheets like this or even full HTML command line reference, something we sorely lack today.
> > > > 
> > > > Maybe this is just dreaming, but I expect YAML would also make it easier to automatically parse enum values to options, like /arch:avx512 etc. We could do that today, but it requires hacking on and extending this janky Record abstraction that's not well understood. Or maybe it just requires effort and motivation.
> > > But this file is effectively duplicating the information about what flags clang-cl supports. Am I the only one that's concerned about how to maintain that, and the two getting out of sync?
> > > 
> > > Olga mentioned (https://reviews.llvm.org/D42762?id=132243#inline-375127) something about extending rules rather than overwriting them. Would that be an option for us, i.e. just exposing the regular cl options and maybe do a few tweaks where really necessary?
> > I am concerned about the duplication, but I think encoding the information in this XML into TableGen is going to be really ugly. Even if we stored this info outside the tablegen, we'd need some way to "join" the property sheet data with the .td data, which effectively duplicates the list of flags, or at least the supported flags with documentation and display names, similar to the way the attribute reference docs work.
> > 
> > Re: Olga's comment, maybe that would work, but I suspect we won't have as much control over the organization.
> extending a rules file only works when you want to add a new category or option, not when you want to remove some options but keep others.  So it's not going to save us from the duplication.  Personally though I'm not at all concerned about the duplication.  If we take the duplicated file as it exists in this CL, and never touch it or maintain it ever again, we would still be providing a better user experience than doing nothing and relying on whatever version happens to ship with Visual Studio.
> 
> It's extremely unlikely that we would //never// touch it again, however.  And in the worst case scenario, if we do ultimately find out that it's too much maintenance, we can change 4 or 5 lines of xml to disable our custom rules file, push an update to the marketplace, and people will automatically get updated to the new version.
"If we take the duplicated file as it exists in this CL, and never touch it or maintain it ever again, we would still be providing a better user experience than doing nothing and relying on whatever version happens to ship with Visual Studio."

I'm not sure I agree, but I think this has been reviewed to exhaustion. Go ahead and submit, please publish under the llvm-vs-code-extensions at google.com account, and I guess we'll see how it works out.

https://reviews.llvm.org/D42762