[PATCH] D42762: Rewrite the VS Integration Scripts

[Zachary Turner via llvm-commits]

I don’t think it’s quite ready for publishing yet (I still need to fix the
/Zi bug for example), but I’ll do that in a followup
On Wed, Feb 7, 2018 at 1:46 AM Hans Wennborg via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180207/556d1caa/attachment.html>