Please test the cases where the result is used; this should be handled for you automatically, but still.

Please test prefix increment/decrement as well.

LGTM

I don't think we can just break ABI for a type like <3 x double> that can currently be produced by frontends like Clang and Swift; being sensitive to -mavx doesn't mean we don't care about the ABI at all. CC'ing @scanon in case he has other thoughts.

LGTM

LGTM.

Thanks, LGTM.

Okay. In that case, I would ask that if you're considering going through the work of adding relocations, please consider adding both scaled and unscaled relative-offset-to-equivalent-symbol.

There's two independently-useful things here for the relocation, right? First, it's useful to be able to express a relocation to a symbol that has the semantics of a particular function but doesn't necessarily have its address, and that concept could be used across many "physical" relocations; and second, it's potentially useful to have a shifted-by-two relative address relocation, at least on AArch64 where instructions (and v-table entries under this ABI) are always four-byte-aligned anyway. Is it possible to separate these concerns in ELF, e.g. by having a "symbol" that can be the target of any other relocation but which actually represents a function of unspecified address with the semantics of another function?

LGTM other than the request to split the commit.

Thanks, this looks right, assuming you've decided to continue calling them "objc blocks" despite my comment.

In D72543#1829870, @steven_wu wrote:

In D72543#1825496, @smeenai wrote:

In D72543#1825454, @rjmccall wrote:

In D72543#1825422, @smeenai wrote:

In D72543#1825178, @rjmccall wrote:

In D72543#1825150, @smeenai wrote:

From spelunking through history, it looks like D33030 tried to solve the same problem, and then rL319123 undid that solution and came up with a different one to solve the same underlying issue. rL319123 is still in place; does it need to be adjusted for the change in this diff?

It looks like those patches were attempts to ensure adequate alignment of the exception object given the absence of an alignment attribute on the _Unwind_Exception type itself. rL319123 became inadequate after _Unwind_Exception became an aligned type because that change still affected the layout of the C++ exception types. What we're fixing here is essentially the same problem that was introduced by D33030.

Makes sense. Should rL319123 be reverted then? The extra space that's being allocated by that change doesn't do what it's meant to do, so it seems wasteful to keep allocating it.

If __cxa_exception is already sufficiently aligned, I believe that patch does not allocate any extra space.

You're right; I understand the case rL319123 was intended for now.

Sorry I was on vacation. I intended to keep rL319123 as least for now. The current setup for MacOS is that clang header is setup to use unwind.h from SDK, which it will currently find the unaligned exception struct. After this patch, rL319123 is a no-op for people building libcxxabi against TOT llvm libunwind but it will help people building against the old header to get the correct alignment fro exception struct.

@rjmccall are you suggesting to put the static_assert directly in the header so users can get build failures if they expect different layout? That can help but I need to figure out what is the correct layout different users are expecting.

LGTM

Remember that the design is that constrained intrinsics must be used whenever *any* code in the function is constrained. It is not unreasonable that part of the function might be constrained and the rest subject to fast-math; it'd be a shame if the intrinsics couldn't even express that.

It wouldn't be a bad idea to track that correspondence in the AST just on the general principle that it's useful information (for diagnostics, tooling, etc.) that's otherwise hard to recreate. But I agree that it's also not problematic to expect Sema to add them in the proper order, and if we don't have an ABI that needs the correspondence directly (e.g. because it wants to emit the operators in pairs), it's fine to rely on it.

I see two reasonable approaches here: we can rely on the implicit members being added in the right order by Sema, as this patch seems to do, or we can broaden the special treatment of implicit virtual functions in the v-table layout code. The latter is more complex (we'd basically need to collect and sort all the implicit virtual functions), but it reliably isolates the ABI from implementation decisions like the order in which Sema processes implicit members and whether some lookup might have triggered an implicit declaration at a point we didn't expect.

In D72543#1825422, @smeenai wrote:

In D72543#1825178, @rjmccall wrote:

In D72543#1825150, @smeenai wrote:

From spelunking through history, it looks like D33030 tried to solve the same problem, and then rL319123 undid that solution and came up with a different one to solve the same underlying issue. rL319123 is still in place; does it need to be adjusted for the change in this diff?

It looks like those patches were attempts to ensure adequate alignment of the exception object given the absence of an alignment attribute on the _Unwind_Exception type itself. rL319123 became inadequate after _Unwind_Exception became an aligned type because that change still affected the layout of the C++ exception types. What we're fixing here is essentially the same problem that was introduced by D33030.

Makes sense. Should rL319123 be reverted then? The extra space that's being allocated by that change doesn't do what it's meant to do, so it seems wasteful to keep allocating it.

In D72543#1825150, @smeenai wrote:

From spelunking through history, it looks like D33030 tried to solve the same problem, and then rL319123 undid that solution and came up with a different one to solve the same underlying issue. rL319123 is still in place; does it need to be adjusted for the change in this diff?

Didn't we work this out already when John added the alignment tracking stuff? I remember this bug involving libjpegturbo standalone assembly receiving a 32-bit argument, and then using the full 64-bit RDI register to read it, but clang stopped zero extending it.

Well, just like for all the other FP builder methods, you can use the setIsFPConstrained method on the builder object to switch between strict and non-strict mode. Does this not suffice, or is there anything particular about the comparisons that would require anything extra?

Okay. This seems conceptually reasonable to me, but I don't feel qualified to review the code, sorry.

Is this approach going to work with scope-local strictness? We need a way to do a comparison that has the non-strict properties but appears in a function that enables strictness elsewhere.

In D72543#1817753, @compnerd wrote:

Do we need to worry about compatibility? What happens if there is a mix between an old binary and a new binary?

Do we need to be careful about the alignment used by __attribute__((aligned)) on different targets? And would be it useful to put the static_asserts in the headers to make it more likely that we catch problems?

In D70172#1812664, @yaxunl wrote:

In D70172#1812631, @rjmccall wrote:

Most uses of the destructor do not use the delete operator, though, and therefore should not trigger the diagnostics in f to be emitted. And this really doesn't require a fully-realized use graph; you could very easily track the current use stack when making a later pass over the entities used.

The call graph is not for this specific situation. A call graph is needed because of the transitive nature of the deferred diagnostic message. That is, if any direct or indirect caller is emitted, the diagnostic msg needs to be emitted.

In D70172#1812533, @yaxunl wrote:

In D70172#1809571, @rjmccall wrote:

I thought you were saying that the destructor decl hadn't been created yet, but I see now that you're saying something more subtle.

CurContext is set to the destructor because the standard says in [class.dtor]p13:

At the point of definition of a virtual destructor (including an implicit definition), the non-array deallocation function is determined as if for the expression `delete this` appearing in a non-virtual destructor of the destructor’s class.

Which is to say that, semantically, the context is as if it were within the destructor, to the extent that this affects access control and so on.

I can see why this causes problems for your call graph (really a use graph), since it's a use in the apparent context of the destructor at a point where the destructor is not being defined. A similar thing happens with default arguments, but because we don't consider uses from default arguments to be true ODR-uses until the default argument is used, that probably doesn't cause problems for you.

I don't think the destructor -> deallocation function edge is actually interesting for your use graph. It'd be more appropriate to treat the deallocation function as used by the v-table than by the destructor; I don't know whether you make any attempt to model v-tables as nodes in your use graph. You might consider finding a simple way to suppress adding this edge, like just not adding edges from a destructor that's not currently being defined (D->willHaveBody()).

With all that said, maintaining a use graph for all the functions you might emit in the entire translation unit seems very expensive and brittle. Have you considered doing this walk in a final pass? You could just build up a set of all the functions you know you're going to emit and then walk their bodies looking for uses of lazy-emitted entities. If we don't already have a function that calls a callback for every declaration ODR-used by a function body, we should.

The deferred diagnostic mechanism is shared between CUDA/HIP and OpenMP. The diagnostic messages not only depend on the callee, but also depend on the caller, the caller information needs to be kept. Also if a caller is to be emitted, all the deferred diagnostics associated with the direct or indirect callees need to be emitted. Therefore a call graph is needed for this mechanism.

If we ignore the dtor->deallocation edge in the call graph, we may miss diagnostics, e.g.

static __device__ __host__ void f(__m256i *p) {
  __asm__ volatile("vmovaps  %0, %%ymm0" ::"m"(*(__m256i *)p)
                 : "r0"); // MS-error{{unknown register name 'r0' in asm}}
}
struct CFileStream {
  void operator delete(void *p) {
    f(0);  // MS-note{{called by 'operator delete'}}
  }
  CFileStream();
  virtual ~CFileStream();  // MS-note{{called by '~CFileStream'}}
};
 
struct CMultiFileStream {
  CFileStream m_fileStream;
  ~CMultiFileStream();
};
 
// This causes vtable emitted so that deleting dtor is emitted for MS.
CFileStream::CFileStream() {}

Assuming the host compilation is on windows.

Here f() is a host device function which is unknown to be emitted, therefore the inline assembly error results in a delayed diagnostic. When f() is checked in the delete operator body, a 'delete operator -> f' edge is added to the call graph since f() is unknown to be emitted.

Since CFileStream::CFileStream is defined, clang sets vtbl to be emitted and does an explicit dtor check even though dtor is not defined. clang knows that this dtor check is for deleting dtor and will check delete operator as referenced, which causes `dtor -> delete operator' to be added to the call graph. Then clang checks dtor as referenced. Since deleting dtor will be emitted together with vtbl, clang should assume dtor is to be emitted. Then clang will found the callees 'delete operator' and f(), and emits the delayed diagnostics associated with them.

If we do not add 'dtor -> delete operator' edge to the call graph, the diagnostic msg in f() will not be emitted.

Thanks, LGTM!

I thought you were saying that the destructor decl hadn't been created yet, but I see now that you're saying something more subtle.

I mean, I made a recommendation and you dismissed it.

It's not unusual for new warnings to require changes to other tests. I agree with enabling this by default.

LGTM

In D50119#1808727, @Quuxplusone wrote:

In D50119#1808616, @rjmccall wrote:

If the committee is actively looking into this problem and considering multiple alternatives, I don't see a particular need to accept your proposal into Clang in advance of the committee's decision. Arguably that would even be somewhat inappropriate, since it might be seen as an endorsement of your proposal over the alternatives, which I don't believe anyone from Clang has looked into. Letting the committee make a decision also addresses our disagreements about the language design and avoids introducing unnecessary divergence if the eventually-accepted design doesn't match your proposal.

Having a workable patch that you've taken advantage of in various projects should count as implementation experience for the committee's purposes.

If the committee *isn't* taking this up, they absolutely should, though.

I see the situation as exactly the opposite of what you just said. We cannot get implementation experience without an implementation. I would like Clang to be that implementation. But if Clang refuses to implement anything that is not Standard C++, then we have a chicken-and-egg problem: nobody can experiment with TR on real codebases until a compiler supports it. I would like Clang to be that compiler.

If the committee *isn't* taking this up, they absolutely should, though.

If the committee is actively looking into this problem and considering multiple alternatives, I don't see a particular need to accept your proposal into Clang in advance of the committee's decision. Arguably that would even be somewhat inappropriate, since it might be seen as an endorsement of your proposal over the alternatives, which I don't believe anyone from Clang has looked into. Letting the committee make a decision also addresses our disagreements about the language design and avoids introducing unnecessary divergence if the eventually-accepted design doesn't match your proposal.

In D72271#1807790, @simon_tatham wrote:

The current return value of EmitTargetBuiltinExpr will be the llvm::Value * corresponding to the last of a sequence of store instructions that writes the constructed aggregate into the return value slot. There's no need to actually use that Value for anything in this case, but we can still test that it's non-null, to find out whether EmitTargetBuiltinExpr has successfully recognized a builtin and generated some code, or whether we have to fall through to the ErrorUnsupported.

In D68101#1806135, @nickdesaulniers wrote:

In D68101#1802220, @bd1976llvm wrote:

Below is the code comment from the new patch explaining the new approach, please take a look and see if you have any questions/comments:

// If two globals with differing sizes end up in the same mergeable
// section that section can be assigned an incorrect entry size. Normally,
// the assembler avoids this by putting incompatible globals into
// differently named sections. However, globals can be explicitly assigned
// to a section by specifying the section name. In this case, if unique
// section names are available (-unique-section-names in LLVM) then we
// bin compatible globals into different mergeable sections with the same name.

Looks good up to here.

// Otherwise, if incompatible globals have been explicitly assigned to section by a
// fine-grained/per-symbol mechanism (e.g. via  _attribute_((section(“myname”)))) then
// we issue an error and the user can then change the section assignment. If the

No bueno. The Linux kernel has code where there are 2D global arrays of different entity sizes explicitly placed in different sections (together). GCC does not error for this, (it doesn't mark the section merge-able), neither should we.

In D68101#1802280, @rjmccall wrote:

I laid out a series of three options before:

• Emit different object-file sections for different mergeability settings.
• Only mark an object-file section as mergeable if all the symbols in it would have the same mergeability settings.
• Stop implicitly using mergeability for "ordinary" sections (i.e. sections other than the string section).

Of the above, I really think #2 is the only complete solution.

In D68101#1804006, @bd1976llvm wrote:

I looked into these today. I think we can do the first of these. I have put a WIP patch up here: https://reviews.llvm.org/D72194. Could you comment on the approach taken?

The solution described in that comment is not acceptable. We are not going to tell users that they cannot assign symbols explicitly to sections because LLVM is unable to promise not to miscompile if they do. It is LLVM's responsibility to correctly compile valid code; enabling mergeability for a section containing unnamed_addr symbols is an optimization, and if it is not safe, it needs to be disabled until we can figure out a way to make it safe.

In D69272#1794630, @sepavloff wrote:

In D69272#1793234, @rjmccall wrote:

In D69272#1792928, @sepavloff wrote:

@hfinkel proposed to use outlining to extract a block with the #pragma to separate function. It could be a basis for a full implementation.

I don't think outlining is a reasonable approach. Outlining has a *lot* of other performance consequences, and we'd have to support arbitrary control flow (e.g. goto) in and out of the outlined function, which adds a lot of frontend complexity in pursuit of something that ought be handled at the optimizer level.

The restriction on using the #pragma on top-level only may be considered as request for 'manual outlining'. User have to extract the piece of code that uses the pragma, solving problems like data passing and control flow.

LGTM.

Okay, LGTM.

Thanks. Could you add some tests that nontemporal directives aren't disturbed by either (1) nested nontemporal directives or (2) nested directives of some other kind?

In D69272#1792928, @sepavloff wrote:

In D69272#1792877, @kpn wrote:

My understanding of this patch is that it only allows the #pragma at the top of each function. It doesn't allow it in blocks inside the function. So if a function has a block inside it that uses strict FP the patch doesn't change the rest of the function to use constrained FP with the settings like you said. And my question was asking if there was a way forward with this patch to a full implementation.

@hfinkel proposed to use outlining to extract a block with the #pragma to separate function. It could be a basis for a full implementation.

Okay, if this is just handling OpenMP-specific control flow and doesn't need to interact with what a reasonable frontend would do with cleanups, I'm appeased.

We do have a static assert. I won't insist on the comment.

In D71650#1790067, @mstorsjo wrote:

In D71650#1789897, @rjmccall wrote:

Wow, that's novel. Please add a comment explaining that this is a compiler workaround, but otherwise LGTM.

The post-patch form doesn't look that odd to me (and we wouldn't want one comment for every one of the existing enums that already are outside of the structs where they are used for bitfield sizes), but do you think a comment is warranted here on this one?

Wow, that's novel. Please add a comment explaining that this is a compiler workaround, but otherwise LGTM.

That's what I figured. The thing is that that necessarily interacts correctly with everything in Clang's IRGen in ways that making a second stack that Clang's IRGen doesn't directly know about doesn't.

So it's never that OpenMP has things that need to be finalized before exiting (e.g. if something in frontend-emitted code throws an exception), it's just that OpenMP might need to generate its own control flow that breaks through arbitrary scopes in the frontend?

In D70258#1788396, @jdoerfert wrote:

In D70258#1788305, @rjmccall wrote:

Introducing an IRBuilder-level finalization stack sounds like it's going to be a huge mess if your goal is to plug this into other frontends.

While I get that you don't want to review this, I would really like to understand why you think this would become a mess.

Okay.

I opposed the creation of this library on these terms in the first place, so I'm pretty sure I'm not on the hook to review it. Introducing an IRBuilder-level finalization stack sounds like it's going to be a huge mess if your goal is to plug this into other frontends.

No, that's not true; something with External linkage would not trip any of those conditions normally. ("Externally initialized" in particular is basically unrelated to linkage.) I think you want a condition something like L->getName() == R->getName() || (L->hasLocalLinkage() && R->hasLocalLinkage()).

Maybe this should depend on linkage?