This makes a lot of sense to me.

LGTM. note to other reviewers, D72998 fixes the max alignment constant in this.

In D72998#1828571, @lebedev.ri wrote:

In D72998#1828559, @hfinkel wrote:

Can we, at least, put this constant in a header file so we don't repeat it in several places?

Remember, we can't use llvm::Value::MaximumAlignment itself in clang sema.
Which header do you have in mind?

Also, can we write it as 0x20000000 so that it's more obvious what the value is.

Sure, will do.

Sorry, I didn't realize you were waiting for me. This seems fine as it is, I don't see the attribute buying us anything.

I'm concerned about just making this fallthrough to 'false'. These ARE integral promotions, we just don't know the type size. What happens if you instantiate this template? Will it still give the correct answer/type/diagnosis?

I don't see anything I have a problem with, but still want @ctopper to have a bit of time to take a look. Ping me monday on both if he doesn't respond.

I think this is alright, I want @ctopper to take a look before I approve it though. Additionally, do you know if this modifies the regcall calling convention at all? Should it?

Ping?

In D71463#1797436, @Anastasia wrote:

It looks good, I was just thinking whether it would be possible to share more common infrastructure. There is Sema::CheckVectorOperands that corresponding OpenCL methods are using internally. Do you think it is possible to share the code more?

@eli.friedman @rjmccall Do you have further comments on these?

@aaron.ballman s comments done.

New patch coming as soon as my build finishes.

Ping :)

In D71467#1786188, @uweigand wrote:

In D71467#1785943, @erichkeane wrote:

I did the compare operators that didn't work right, and will do a separate patch for the fp-classification type ones: f02d6dd6c7afc08f871a623c0411f2d77ed6acf8

Thanks! Now I'm getting the correct output for the float test cases as well, and I've added them to the patch.

As to fp-classification, I think there is an additional complication here: according to IEEE and the proposed C2x standard, these builtins should never raise any exception, not even when receiving a signaling NaN as input. Strictly speaking, this means that they cannot possibly be implemented in terms of any comparison operation.

Now, on SystemZ (and many other platforms, I think) there are in fact specialized instructions that will implement the required semantics without raising any exceptions, but there seems to be no way to represent those at the LLVM IR level. We'll probably need some extensions here (some new IR-level builtins?) ...

(But I'd say that problem is unrelated to this patch, so I'd prefer to decouple that problem from the question of whether this patch is the right solution for comparisons.)

In D71467#1784398, @erichkeane wrote:

In D71467#1784338, @craig.topper wrote:

In D71467#1784286, @rjmccall wrote:

The bug with __builtin_isless should be a really easy fix; the builtin just needs to be flagged as having custom type-checking, and then we need to make sure we do appropriate promotions on the arguments (but we probably do).

I think I convinced @erichkeane to look at it on Monday.

Everything but fpclassify is pretty trivial, I just needed to write a test but needed to go home. I'll commit that Monday when I get to it. Fpclassify will take a touch longer since the int arguments need to be dealt with, but that shouldn't be more than a little work.

In D71467#1784338, @craig.topper wrote:

In D71467#1784286, @rjmccall wrote:

The bug with __builtin_isless should be a really easy fix; the builtin just needs to be flagged as having custom type-checking, and then we need to make sure we do appropriate promotions on the arguments (but we probably do).

I think I convinced @erichkeane to look at it on Monday.

Rebase + @eli.friedman s comments.

In D71463#1783953, @efriedma wrote:

Can you update https://clang.llvm.org/docs/LanguageExtensions.html#langext-vectors with a description of the interaction between the different language features here?

Sorry in advance for the reviewer choices... Blame got me you all for all touching these things in the early 2010s. Since then, Richard and I seem to be the only one to have touched these types.

I looked at the test failure, the REQUIRES line is necessary because the test calls emit-obj. I've asked Zahira to commit it to fix the bots.

Should this be in a namespace? Can't tell from the header file, is it at least in clang?

In D69950#1771462, @eandrews wrote:

In D69950#1771340, @mstorsjo wrote:

In D69950#1771133, @eandrews wrote:

In D69950#1770138, @mstorsjo wrote:

This (when reapplied in https://reviews.llvm.org/rG878a24ee244a24c39d1c57e9af2) broke compilation of code that earlier built fine. A reduced example:

namespace glslang {
class TPoolAllocator {
  void operator=(TPoolAllocator);
};
template <class> class a {
  TPoolAllocator *b;
  void c() { allocator = *b; }
  TPoolAllocator allocator;
};
} // namespace glslang

With this patch, some errors in templates are diagnosed earlier (i.e. does not wait till instantiation). Since 'allocator' and 'b' aren't dependent, I think this is a valid diagnosis. GCC throws an error on this code upon instantiation. https://godbolt.org/z/X9Y-Vy

The original non-reduced case does build fine with GCC though (I'm fairly sure). I can try to reduce one later that still builds with GCC but fails with current clang.

Yes. The reduced example here builds fine with GCC as well, unless template is instantiated. However I don't think Clang always matches GCC behavior when diagnosing errors in templates (See example from cpplearner here - https://reviews.llvm.org/D61027). Clang diagnoses several errors even if template is not instantiated (unlike GCC). I am not sure what the 'correct' behavior is / whether we should match GCC here. @erichkeane @rnk could you please take a look?

In D62731#1769532, @thakis wrote:

The tests fail on Windows, http://45.33.8.238/win/3405/step_6.txt

Please take a look, and if it takes a while to fix please revert while you investigte.

Otherwise I think I'm OK. @thegameg ?

Microsoft recently released 16.4 and has ceased support for the 16.3 line, so it isn't really important to me anymore. I'd thought this was a 'hold our nose and do it' type fix, but I don't think that is justified any longer now that Microsoft doesn't even support it.

unique_ptr already has a 'bool' builtin to its state (one that this code checks later anyway), so there isn't a good reason to return a bool here. Otherwise I think I'm OK with this.

In D68050#1763191, @fhahn wrote:

Hi Eric,

Thanks for putting up the initial patch. Are you interested in pushing it forward? Or would you prefer someone else taking over?

Cheers,
Florian

In D70791#1762155, @rnk wrote:

My version of cl.exe claims to have version 19.23.28107 and my headers are in Tools/MSVC/14.23.28105/include, and I see that this is fixed in xtr1common. is_integral_v and the helpers are defined with implicit template instantiations. There are no fully specialized template definitions in that header. So, my hope is that this problem will go away as users run the VS updater. Although, for buildbots and users such as myself who rarely start the IDE, that may take longer than we'd like.

In D70791#1762081, @rnk wrote:

I thought we agreed to remove the workaround that we had for this and just declare that the old MSVC STL release was buggy and users should update:
https://bugs.llvm.org/show_bug.cgi?id=42027
It's a point release that preserves ABI compatibility, so there's less motivation for us to add a compat hack.

In general, we should seriously think about sunsetting some MS compat hacks that aren't needed for new SDK/STL versions to save on clang maintenance costs. The complexity they add is surprisingly expensive. For example, -fdelayed-template-parsing is a recurring source of compat issues in clang-tools-extra (see the commit logs), and I would like to revive D66394 to get it disabled by default soon. (+@thakis)

I haven't looked at the tests because I don't terribly understand the sanitizer IR (hopefully someone else can take a look), but the logic/motivation seems solid to me.

In D68824#1733621, @hans wrote:

We hit this in V8 which is annotating some pointers as being 4GB-aligned. Does anyone know what it would take to raise Clang's limit here?

In D68824#1704942, @rsmith wrote:

This seems to be missing a CodeGen test for what IR we generate on an overly-large alignment. (The warning says the alignment is ignored, but I don't see where you're actually doing anything different in that case when generating IR.)

cant use llvm::Value in Sema without some linker issues.

Hal's comments

Remove unused code

Ah, Ugg... GCC must implement the target parsing handling different on a per-architecture basis (ARM vs X86). We've only got parsing in place to do the x86 way. That ends up making this a much more difficult thing to maintain I suspect.

Seems your test changes have disappeared as well? Otherwise, 1 comment then I'm ok with this.

Also, can you add a test for GNUCmpXchg in both situations (inside a template, and outside)? It is the most complicated, and would best reflect the issue.

It kinda stinks that we have to do this at such a late step. I'd have much preferred doing this as a part of the rebuild, but it appears that the 'form' takes quite a bit to calculate. Also, I'd likely have preferred that the initial reordering happened in codegen instead of this early, though perhaps thats too large of a change to do here.

Yikes, good catch!