[PATCH] D62046: [OpenMP][bugfix] Add missing math functions variants for log and abs.

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

I'm not sure about this diff. I think it's breaking  and . 
Raised bug https://bugs.llvm.org/show_bug.cgi?id=42972


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62046/new/

https://reviews.llvm.org/D62046



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

In D64943#173 , @ABataev wrote:

> In D64943#158 , @JonChesterfield 
> wrote:
>
> > > OpenMP linker script is known to cause problems for gold and lld linkers 
> > > on Linux and it will also cause problems for Windows enabling in future
> >
> > What are the known problems with the linker script? I'm wondering if they 
> > can be resolved without the overhead of introducing a new tool.
>
>
> They just do not support linker script. And, thus, cannot be used for 
> offloading. Only `ld` supports it.


In what respect? I've used linker scripts with both gold and lld, and both 
instances of --help text claim to support them. In the case of lld, a very 
complicated script hit a few internal errors, but I believe they've all been 
fixed since.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64943/new/

https://reviews.llvm.org/D64943



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

> OpenMP linker script is known to cause problems for gold and lld linkers on 
> Linux and it will also cause problems for Windows enabling in future

What are the known problems with the linker script? I'm wondering if they can 
be resolved without the overhead of introducing a new tool.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64943/new/

https://reviews.llvm.org/D64943



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

In D64943#179 , @ABataev wrote:

> In D64943#178 , @JonChesterfield 
> wrote:
>
> > In D64943#173 , @ABataev wrote:
> >
> > > In D64943#158 , 
> > > @JonChesterfield wrote:
> > >
> > > > > OpenMP linker script is known to cause problems for gold and lld 
> > > > > linkers on Linux and it will also cause problems for Windows enabling 
> > > > > in future
> > > >
> > > > What are the known problems with the linker script? I'm wondering if 
> > > > they can be resolved without the overhead of introducing a new tool.
> > >
> > >
> > > They just do not support linker script. And, thus, cannot be used for 
> > > offloading. Only `ld` supports it.
> >
> >
> > In what respect? I've used linker scripts with both gold and lld, and both 
> > instances of --help text claim to support them. In the case of lld, a very 
> > complicated script hit a few internal errors, but I believe they've all 
> > been fixed since.
>
>
> Hmm, I tried it with gold some time ago and it just did not work for me. The 
> linking failed with diagnostics that some of the commands in the script are 
> unknown.


The problem turns out to be the 'insert before' statement. ld and lld support 
it, gold does not. According to 
https://bugzilla.redhat.com/show_bug.cgi?id=927573, the recommended workaround 
is essentially that implemented in this differential.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64943/new/

https://reviews.llvm.org/D64943



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

In D64943#1666849 , @sdmitriev wrote:

> In D64943#136 , @JonChesterfield 
> wrote:
>
> > I'm not sure copying the crtbegin/crtend mechanism from the early days of C 
> > runtime is ideal. Since the data is stored in a common section anyway, 
> > please could we rename it to __omp_offloading_entries in which case the 
> > linker will provide start/end symbols automatically?
>
>
> Well, I never said that it is an ideal solution, but it is a known mechanism 
> that works well in many cases and can also be reused for the offloading entry 
> table.
>  I do not fully understand your suggestion for renaming entries section, how 
> it will help with providing start/end symbols for the entries. Can you please 
> provide more details?


Given a custom elf section with a C identifier as a name, the linker will 
provide definitions of `__start_name`/`__stop_name` to satisfy unresolved 
symbols. I don't believe this occurs if the section name is not a C identifier, 
e.g. contains a period. So unless I've misinterpreted the purpose of the two 
object files, they can be removed in exchange for renaming the section.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64943/new/

https://reviews.llvm.org/D64943



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

I'm not sure copying the crtbegin/crtend mechanism from the early days of C 
runtime is ideal. Since the data is stored in a common section anyway, please 
could we rename it to __omp_offloading_entries in which case the linker will 
provide start/end symbols automatically? That removes the two object files and 
the link order dependency which is a hazard to bitcode libraries.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64943/new/

https://reviews.llvm.org/D64943



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

> Hm, I was not aware of this Linux linker feature, thanks a lot for the 
> explanation! I see only one problem with using it as a replacement for the 
> begin/end objects – it looks like `__start_name`/`__stop_name` symbols are 
> created with `default` visibility instead of `hidden`. I guess it will cause 
> problems for offload programs that use shared libraries because DSO’s 
> `__start_name`/`__stop_name` symbols will be preempted by the executable’s 
> symbols and that is not what we want. Is there any way to change this 
> behavior?

Declaring the symbol as `__attribute__((__visibility__("hidden")))` just works 
as far as I can tell. The linker still provides the right definition, objdump 
says it's hidden.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64943/new/

https://reviews.llvm.org/D64943



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

I'm on board with getting rid of the linker script. Gold's limited support for 
that seems conclusive.

I believe the current script does two things:
1/ takes a binary and embeds it in a section named 
.omp_offloading.amdgcn-amd-amdhsa
2/ provides start/end symbols for that section and for .omp_offloading.entries.

2/ is discussed above.
1/ can be implemented as a call to (llvm-)objcopy

> If binary is used as the value for --input-target, the input file will be 
> embedded as a data section in an ELF relocatable object, with symbols 
> _binary__start, _binary__end, and 
> _binary__size representing the start, end and size of the data, 
> where  is the path of the input file as specified on the command 
> line with non-alphanumeric characters converted to _.

I think dropping the linker script means that cmake will need to invoke an 
extra executable. As far as I can see, that tool can be objcopy instead of 
clang-offload-wrapper.

Does this diff mix getting rid of the linker script with other changes? E.g. it 
looks like the metadata generation is moving from clang to the new tool, but 
that seems orthogonal to dropping the linker script.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64943/new/

https://reviews.llvm.org/D64943



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

> I see some problems with using llvm-objcopy for that. First issue is that 
> symbols created by llvm-objcopy for embedded data depend on the input file 
> name. As you know these symbols are referenced from the offload registration 
> code that is currently added to an object by the clang at compile time. I not 
> sure how you can guarantee that symbol names will match.

That seems solvable by renaming the input file / passing a string to clang.

> And another, more important problem is that it won't work on Windows because 
> llvm-objcopy produces ELF object according to the description.

objcopy works with coff in the meantime, and we already need a bunch of unix 
tools to build llvm on windows.

> Anyway I am going to change entries section name to "omp_offloading_entries", 
> remove omptargetbegin.o/omptargetend.o and upload the revised patch.

Thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64943/new/

https://reviews.llvm.org/D64943



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

>> Does this diff mix getting rid of the linker script with other changes? E.g. 
>> it looks like the metadata generation is moving from clang to the new tool, 
>> but that seems orthogonal to dropping the linker script.
> 
> Metadata is still generated by the clang, there are no changes in this area. 
> What is moving to a wrapper tool is the generation of the offload 
> registration code. Let me just attach the slides that I presented on the 
> inter company meeting were the proposal was discussed. It'll probably answer 
> most of your questions. F9983224: openmp_linker_script.pdf 
> 

It does indeed, thanks. I see the motivation for delaying offload registration 
code. I'm pretty sure that is indeed orthogonal to removing the linker script.

How would you feel about using objcopy to embed the device binary?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64943/new/

https://reviews.llvm.org/D64943



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68166: [Clang][OpenMP Offload] Add new tool for wrapping offload device binaries

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

I still don't understand what advantage the standalone tool has over renaming 
the file to `omp_offloading` and then using `objcopy -I binary`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68166/new/

https://reviews.llvm.org/D68166



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68166: [Clang][OpenMP Offload] Add new tool for wrapping offload device binaries

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

I think this patch is a behaviour change. Currently, the target binary is 
embedded in the host binary at link time. With this change, the contents of the 
binary are embedded in bitcode which is subsequently fed into the link. If 
indeed so, that seems strictly better - code in the host that cares about the 
size of the bitcode now has it available at opt time, instead of at link time. 
The target specific nastiness objcopy would introduce is neatly sidestepped.

This change takes N binaries (that I think need to be for different triples, or 
the loop doesn't work) and puts them in separate section-annotated bitcode 
arrays. Equivalent behaviour would result from calling the tool once per binary 
and passing the N results onward, e.g. to llvm-link.

The functionality of 'take a binary and embed it in bitcode as a const array' 
is likely to be useful outside of openmp. I've done similar things in the past 
in non-portable fashion. Aside from the section and symbol names, I don't think 
there's anything specific to openmp in the tool.

How would you feel about simplifying the tool to work on one file at a time, 
with an interface that takes the host target (could default to whatever is 
running the tool) and a string for section name, which generates some bitcode 
containing that file as a const array plus start/end symbols derived from the 
section name? The change would involve deleting the multiple file handling and 
renaming OffloadTargets to SectionName or similar.

clang-offload-wrapper than becomes binary-to-bitcode-embedder (or better, names 
are hard), with the intent that projects outside of the openmp target offload 
compiler could use it.




Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:84
+  void createImages(ArrayRef Binaries) {
+for (const BinaryDesc  : Binaries) {
+  StringRef SectionName = SS.save(".omp_offloading." + Bin.second);

I don't think this works for multiple binaries with the same target triple. 
They'll all be put in the same section and there will be duplicate symbols for 
start/end.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68166/new/

https://reviews.llvm.org/D68166



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1643
   HelpText<"Emit OpenMP code only for SIMD-based constructs.">;
+def fopenmp_new_codegen : Flag<["-"], "fopenmp-new-codegen">, Group, 
Flags<[CC1Option, NoArgumentUnused, HelpHidden]>,
+  HelpText<"Use the experimental OpenMP-IR-Builder codegen path.">;

jdoerfert wrote:
> ABataev wrote:
> > Maybe just `-fopenmp-experimental`?
> I would prefer the option to be self explanatory but I'm not married to the 
> current name.
`enable-openmpirbuilder?`



Comment at: llvm/include/llvm/IR/OpenMPKinds.def:186
+///{
+
+#ifndef OMP_IDENT_FLAG

Sharing constants between the compiler and the runtime is an interesting 
subproblem. I think the cleanest solution is the one used by libc, where the 
compiler generates header files containing the constants which the runtime 
library includes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69785/new/

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: clang/test/OpenMP/barrier_codegen.cpp:22
+// CLANGCG-NOT: readonly
+// IRBUILDER:  ; Function Attrs: nofree nosync nounwind readonly
+// IRBUILDER-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*)

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > jdoerfert wrote:
> > > > > > ABataev wrote:
> > > > > > > jdoerfert wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > Not sure about correct use of `nosync` and `readonly` 
> > > > > > > > > attributes. OpenMP runtime uses lazy initialization of the 
> > > > > > > > > runtime library and when any runtime function is called, the 
> > > > > > > > > inner parts of the OpenMP runtime are initialized 
> > > > > > > > > automatically. It may use some sync primitives and may modify 
> > > > > > > > > memory, I assume. Same about `nofree`.
> > > > > > > > There are two versions of these functions, host and device. I 
> > > > > > > > assume host functions are not inlined but device functions 
> > > > > > > > might be. This is basically all the modes we support right now.
> > > > > > > > 
> > > > > > > > If we do not inline the function (host) we don't necessarily 
> > > > > > > > care what they do but what effect the user can expect.
> > > > > > > > The user can not expect to synchronize through 
> > > > > > > > `__kmpc_global_thread_num` calls in a defined way, thus 
> > > > > > > > `nosync`.
> > > > > > > > Similarly, from the users perspective there is no way to 
> > > > > > > > determine if something was written or freed, no matter how many 
> > > > > > > > of these calls I issue and under which control conditions, all 
> > > > > > > > I see is the number as a result. Thus, `readonly` and `nofree`. 
> > > > > > > > I believe `readnone` is even fine here but it might not work 
> > > > > > > > for the device version (see below) so I removed it.
> > > > > > > > 
> > > > > > > > If we do inline the function (device) we need to make sure the 
> > > > > > > > attributes are compatible with the inlined code to not cause 
> > > > > > > > UB. The code of `__kmpc_global_thread_num` at least does not 
> > > > > > > > write anything and only reads stuff (as far as I can tell).
> > > > > > > > 
> > > > > > > > Please correct me if I overlooked something. 
> > > > > > > But someone may try to inline the host-based runtime or try to 
> > > > > > > use LTO with it.
> > > > > > > The question is not about the user expectations but about the 
> > > > > > > optimizations which can be triggered with these attributes.
> > > > > > This is our runtime and we have supported and unsupported usage 
> > > > > > models.
> > > > > Hmm, I don't think this the right approach. Plus, you still did not 
> > > > > answer about optimizations. Maybe, currently, these attributes won't 
> > > > > trigger dangerous optimizations but they can do this in the future 
> > > > > and it may lead to unpredictable results. I would use the pessimistic 
> > > > > model here rather than over-optimistic.
> > > > I did (try to) describe why there cannot be any problems wrt. 
> > > > optimizations:
> > > > The specified behavior of the runtime call is _as if_ it is `readonly`, 
> > > > `nofree`, and `nosync`.
> > > > That is, from the perspective of the compiler this is true and 
> > > > optimizations are allowed to use that fact.
> > > >  
> > > > The fact that the first ever runtime call initializes the runtime is 
> > > > neither part of the specification nor of the observable behavior. If we 
> > > > change the order between two call to `__kmpc_global_thread_num`, or 
> > > > similar calls, we cannot observe if/which one initialized the runtime 
> > > > and which read only stuff.
> > > Here is the code of this function from the libomp:
> > > ```
> > >   int gtid;
> > > 
> > >   if (!__kmp_init_serial) {
> > > gtid = KMP_GTID_DNE;
> > >   } else
> > > #ifdef KMP_TDATA_GTID
> > >   if (TCR_4(__kmp_gtid_mode) >= 3) {
> > > KA_TRACE(1000, ("*** __kmp_get_global_thread_id_reg: using TDATA\n"));
> > > gtid = __kmp_gtid;
> > >   } else
> > > #endif
> > >   if (TCR_4(__kmp_gtid_mode) >= 2) {
> > > KA_TRACE(1000, ("*** __kmp_get_global_thread_id_reg: using keyed 
> > > TLS\n"));
> > > gtid = __kmp_gtid_get_specific();
> > >   } else {
> > > KA_TRACE(1000,
> > >  ("*** __kmp_get_global_thread_id_reg: using internal 
> > > alg.\n"));
> > > gtid = __kmp_get_global_thread_id();
> > >   }
> > > 
> > >   /* we must be a new uber master sibling thread */
> > >   if (gtid == KMP_GTID_DNE) {
> > > KA_TRACE(10,
> > >  ("__kmp_get_global_thread_id_reg: Encountered new root 
> > > thread. "
> > >   "Registering a new gtid.\n"));
> > > __kmp_acquire_bootstrap_lock(&__kmp_initz_lock);
> > > if (!__kmp_init_serial) {
> > >   __kmp_do_serial_initialize();
> > >   gtid = 

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: llvm/include/llvm/IR/OpenMPKinds.def:186
+///{
+
+#ifndef OMP_IDENT_FLAG

jdoerfert wrote:
> Meinersbur wrote:
> > jdoerfert wrote:
> > > JonChesterfield wrote:
> > > > Sharing constants between the compiler and the runtime is an 
> > > > interesting subproblem. I think the cleanest solution is the one used 
> > > > by libc, where the compiler generates header files containing the 
> > > > constants which the runtime library includes.
> > > I'd prefer not to tackle this right now but get this done first and 
> > > revisit the issue later. OK?
> > I don't think this is a good solution. It means that libomp cannot built 
> > built anymore without also building clang. Moreover, the values cannot be 
> > changed anyway since it would break the ABI.
> > 
> > I'd go the other route: The libomp defines what it's ABI is, the compiler 
> > generates code for it. 
> This patch doesn't change what we do, just where. The numbers are hard coded 
> in clang now. Let's start a discussion on the list and if we come up with a 
> different scheme we do it after this landed.
Revisit later sounds good.

@Meinersbur Do you know of an example of a non-llvm compiler using this libomp?

The usual order is build a compiler, then use it to build the runtime 
libraries, then the whole package can build other stuff. Provided the compiler 
doesn't need any of the runtime libraries (compiler-rt, maths libraries, libomp 
etc) itself the system bootstraps cleanly. Especially important when cross 
compiling and I suspect the gpu targets in openmp have similarly strict 
requirements on the first compiler.

Closely related to that, I tend to assume that the runtime libraries can be 
rewritten to best serve their only client - the associated compiler - so if 
libomp is used by out of tree compilers I'd like to know who we are at risk of 
breaking.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69785/new/

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4011
   unsigned NamedModifiersNumber = 0;
-  SmallVector FoundNameModifiers(
-  OMPD_unknown + 1);
+  SmallVector
+  FoundNameModifiers(unsigned(OMPD_unknown) + 1);

I wonder if it would be worth wrapping the accesses to FoundNameModifiers in 
functor that does the enum class to unsigned conversion. E.g. a class instance 
that contains the small vector and exposes operator[] that takes the enum class.

FoundNameModifiers[unsigned(val)] is quite a lot of line noise.



Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:11
+
+#include "llvm/IR/OpenMPConstants.h"
+

Implemented in OpenMPConstants.cpp instead? Functions look usable independent 
of MPIRBuilder.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69853/new/

https://reviews.llvm.org/D69853



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69498: IR: Invert convergent attribute handling

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

Requiring the presence of an attribute for correctness is a bad thing. OpenMP 
was missing this annotation in a number of places and is probably still missing 
it elsewhere. I wouldn't bet on handwritten bitcode libraries getting it right 
everywhere either. An optimisation pass accidentally dropping the attribute 
seems a plausible failure mode as well.

Strongly in favour of replacing convergent with no{n}convergent in the IR.

Not as convinced it should be inserted by the front end. The attribute is 
needed before any CFG rewrites and as far as I know they all occur downstream 
of the front end. That suggests an IR pass that walks over all functions, 
intrinsic calls, inline asm and so forth and marks them as appropriate. 
Standard C++/x86 and similar don't need to run the pass, OpenCL/x86 probably 
does. I'd suggest running it manually across handwritten bitcode as a sanity 
check as well.

Of course, //if// a front end targeting gpus wants to change control flow 
(julia may do this, mlir does, I sincerely hope clang doesn't) //and// use this 
attribute to control the process, then that front end picks up the 
responsibility for inserting the attribute where it wants it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69498/new/

https://reviews.llvm.org/D69498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

Uncertainty over the handling of constant data between clang and libopenmp not 
withstanding, I think this is good to go.




Comment at: llvm/include/llvm/IR/OpenMPKinds.def:186
+///{
+
+#ifndef OMP_IDENT_FLAG

Meinersbur wrote:
> JonChesterfield wrote:
> > jdoerfert wrote:
> > > Meinersbur wrote:
> > > > jdoerfert wrote:
> > > > > JonChesterfield wrote:
> > > > > > Sharing constants between the compiler and the runtime is an 
> > > > > > interesting subproblem. I think the cleanest solution is the one 
> > > > > > used by libc, where the compiler generates header files containing 
> > > > > > the constants which the runtime library includes.
> > > > > I'd prefer not to tackle this right now but get this done first and 
> > > > > revisit the issue later. OK?
> > > > I don't think this is a good solution. It means that libomp cannot 
> > > > built built anymore without also building clang. Moreover, the values 
> > > > cannot be changed anyway since it would break the ABI.
> > > > 
> > > > I'd go the other route: The libomp defines what it's ABI is, the 
> > > > compiler generates code for it. 
> > > This patch doesn't change what we do, just where. The numbers are hard 
> > > coded in clang now. Let's start a discussion on the list and if we come 
> > > up with a different scheme we do it after this landed.
> > Revisit later sounds good.
> > 
> > @Meinersbur Do you know of an example of a non-llvm compiler using this 
> > libomp?
> > 
> > The usual order is build a compiler, then use it to build the runtime 
> > libraries, then the whole package can build other stuff. Provided the 
> > compiler doesn't need any of the runtime libraries (compiler-rt, maths 
> > libraries, libomp etc) itself the system bootstraps cleanly. Especially 
> > important when cross compiling and I suspect the gpu targets in openmp have 
> > similarly strict requirements on the first compiler.
> > 
> > Closely related to that, I tend to assume that the runtime libraries can be 
> > rewritten to best serve their only client - the associated compiler - so if 
> > libomp is used by out of tree compilers I'd like to know who we are at risk 
> > of breaking.
> > Do you know of an example of a non-llvm compiler using this libomp?
> 
> [[ 
> https://github.com/llvm-project/llvm-project/blob/master/openmp/runtime/src/kmp_gsupport.cpp
>  | gcc  ]] (using libomp's gomp compatibility layer), [[ 
> https://www.openmprtl.org/ | icc  ]] (as libomp was initially donated by 
> Intel).
> 
> I don't understand why it even matters if there are other compilers using 
> libomp. Every LLVM runtime library can be built stand-alone. 
> With constant values being determined during compiler bootstrapping, programs 
> built on one computer would be potentially ABI-incompatible with a runtime 
> library on another. Think about updating your compiler-rt/libomp/libc++ on 
> you computer causing all existing binaries on the system to crash because 
> constants changed in the updated compiler's bootstrapping process.
> 
> The only use case I know that does this is are operating system's syscall 
> tables. Linux's reference is [[ 
> https://github.com/torvalds/linux/blob/master/arch/sh/include/uapi/asm/unistd_64.h
>  | unistd.h ]] which is platform-specific and Windows generates the table 
> during its [[ https://j00ru.vexillium.org/syscalls/nt/64/ | build process ]]. 
> Therefore on Windows, system calls can only be done through ntdll. Even on 
> Linux one should use the system's libc instead of directly invoking a system 
> call.
Thanks. GCC and ICC would presumably be happier with the magic numbers stored 
with openmp then (though with the move to a monorepo that's a little less 
persuasive).

When constants that affect the ABI change, the result won't work with existing 
software regardless of whether the compiler or the library contains the change. 
Either the new compiler builds things that don't work with the old library, or 
the new library doesn't work with things built by the old compiler. The two 
have to agree on the ABI.

At present, openmp does the moral equivalent of #include OpenMPKinds.def from 
clang. Moving the constants to libomp means clang will do the equivalent of 
#include OpenMPKinds.def from openmp. Breaking that dependency means making a 
new subproject that just holds/generates the constants, that both depend on, 
which seems more hassle than it's worth. 

I'd like to generate this header as part of the clang build (though ultimately 
don't care that much if it's generated as part of the openmp build) because 
it's going to become increasingly challenging to read as non-nvptx 
architectures are introduced. Likewise it would be useful to generate the 
interface.h for deviceRTL (or equivalently a set of unit tests checking the 
function types) from the same source to ensure it matches and that's not 
economically feasible within the C preprocessor.


Repository:
  rG LLVM 

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

Great to see the fragile math.h stuff disappear.

I'm not sure about the CPU/GPU/other granularity. An openmp program with x86 as 
the host and target offload regions for amdgcn and for nvptx seems like a 
reasonable aspiration. Or for a couple of different generations from the same 
vendor.

More ambitiously, one might want a GPU to be the host, and offload kernels for 
I/O to an aarch64 "target".

We don't need to wire such combinations in up front, and I don't think they're 
excluded by this design. A future 'x86-64' variant would presumably be chosen 
over a 'cpu' variant when compiling for x86-64.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71179/new/

https://reviews.llvm.org/D71179



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: clang/lib/Headers/__clang_cuda_cmath.h:70
 __DEVICE__ float fmod(float __x, float __y) { return ::fmodf(__x, __y); }
-// TODO: remove when variant is supported
-#ifndef _OPENMP

jdoerfert wrote:
> As far as I can tell, `fpclassify` is not available in CUDA so it is unclear 
> if we want to have it here or not. I removed it due to the TODO above. 
> Consequently I also had to remove other `fpclassify` occurrences. If it turns 
> out the host version is not usable on the device and we need the builtins, we 
> add them back but under the opposite guard, that is `#ifdef _OPENMP`.
We could call __builtin_fpclassify for nvptx, e.g. from 
https://github.com/ROCm-Developer-Tools/aomp-extras/blob/0.7-6/aomp-device-libs/libm/src/libm-nvptx.cpp

```int fpclassify(float __x) {
  return __builtin_fpclassify(FP_NAN, FP_INFINITE, FP_NORMAL, FP_SUBNORMAL, 
FP_ZERO, __x);
}
int fpclassify(double __x) {
  return __builtin_fpclassify(FP_NAN, FP_INFINITE, FP_NORMAL, FP_SUBNORMAL, 
FP_ZERO, __x);
}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71179/new/

https://reviews.llvm.org/D71179



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: clang/test/OpenMP/begin_declare_variant_codegen.cpp:71
+}
+
+// Make sure all ompvariant functions return 1 and all others return 0.

The name mangling should probably append the device kind, .e.g. 
`_Z3foov.ompvariant.gpu`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71179/new/

https://reviews.llvm.org/D71179



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

In D71241#1782846 , @ABataev wrote:

> But I suggest to discuss this with Richard Smith.


Is the appeal to authority necessary to resolve this? The last few posts by Hal 
look clear to me. Especially convincing is:

> We're simply resolving the callee according to the language rules.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71241/new/

https://reviews.llvm.org/D71241



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

Lowering in sema or in codegen seems a standard phase ordering choice. There 
will be pros and cons to both.

I think prior art leans towards sema. Variants are loosely equivalent to tag 
dispatching or constexpr if, both handled before lowering the AST to IR.

Writing the dispatch lowering on IR should make it easier to call from a 
Fortran front end. I'm in favour of minimising work done on the clang AST on 
general principles.

Given we have two implementations, each at different points in the pipeline, it 
might be constructive to each write down why you each choose said point. I 
suspect the rationale is hidden by the implementation details.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71241/new/

https://reviews.llvm.org/D71241



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

Has anyone actually asked Richard to look at this? He isn't subscribed to the 
diff and may not be watching openmp-dev.

I don't think it's reasonable to stall progress on optimising openmp 
indefinitely. Richard may find it difficult to find time to resolve this. Would 
you accept a time out of a week, after which the majority vote carries it?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71241/new/

https://reviews.llvm.org/D71241



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70289: [OpenMP][NFCI] Use the libFrontend ProcBindKind in Clang

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield accepted this revision.
JonChesterfield added a comment.
This revision is now accepted and ready to land.

Nice cleanup, thanks


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70289/new/

https://reviews.llvm.org/D70289



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

> Explain that you're replacing the function written by the user on the fly by 
> another one. If they accept it, go ahead.

That's the observational effect of variants. Replacing is very similar to 
calling + inlining.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71241/new/

https://reviews.llvm.org/D71241



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

In D71241#1782427 , @ABataev wrote:

> In D71241#1782425 , @JonChesterfield 
> wrote:
>
> > > Explain that you're replacing the function written by the user on the fly 
> > > by another one. If they accept it, go ahead.
> >
> > That's the observational effect of variants. Replacing is very similar to 
> > calling + inlining.
>
>
> Not in the AST.


I don't see much difference between mutating the AST and mutating the SSA. 
What're your objections to the former, specifically? It's not a stable 
interface so tools hanging off it will have a process for updating when it 
changes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71241/new/

https://reviews.llvm.org/D71241



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

> https://clang.llvm.org/docs/InternalsManual.html#the-ast-library
> 
>   Faithfulness¶
>   The AST intends to provide a representation of the program that is faithful 
> to the original source. 

That's pretty convincing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71241/new/

https://reviews.llvm.org/D71241



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71103: [libomptarget][nfc] Move three more files to common

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield updated this revision to Diff 232557.
JonChesterfield added a comment.

- update comment


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71103/new/

https://reviews.llvm.org/D71103

Files:
  openmp/libomptarget/deviceRTLs/common/src/parallel.cu
  openmp/libomptarget/deviceRTLs/common/src/support.cu
  openmp/libomptarget/deviceRTLs/common/src/sync.cu
  openmp/libomptarget/deviceRTLs/nvptx/CMakeLists.txt
  openmp/libomptarget/deviceRTLs/nvptx/src/parallel.cu
  openmp/libomptarget/deviceRTLs/nvptx/src/support.cu
  openmp/libomptarget/deviceRTLs/nvptx/src/sync.cu


Index: openmp/libomptarget/deviceRTLs/nvptx/CMakeLists.txt
===
--- openmp/libomptarget/deviceRTLs/nvptx/CMakeLists.txt
+++ openmp/libomptarget/deviceRTLs/nvptx/CMakeLists.txt
@@ -57,10 +57,10 @@
   src/libcall.cu
   ${devicertl_common_directory}/src/loop.cu
   ${devicertl_common_directory}/src/omptarget.cu
-  src/parallel.cu
+  ${devicertl_common_directory}/src/parallel.cu
   src/reduction.cu
-  src/support.cu
-  src/sync.cu
+  ${devicertl_common_directory}/src/support.cu
+  ${devicertl_common_directory}/src/sync.cu
   ${devicertl_common_directory}/src/task.cu
   )
 
Index: openmp/libomptarget/deviceRTLs/common/src/sync.cu
===
--- openmp/libomptarget/deviceRTLs/common/src/sync.cu
+++ openmp/libomptarget/deviceRTLs/common/src/sync.cu
@@ -1,4 +1,4 @@
-//=== sync.h - NVPTX OpenMP synchronizations - CUDA 
-*-===//
+//=== sync.cu - OpenMP synchronizations -- CUDA 
-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
Index: openmp/libomptarget/deviceRTLs/common/src/support.cu
===
--- openmp/libomptarget/deviceRTLs/common/src/support.cu
+++ openmp/libomptarget/deviceRTLs/common/src/support.cu
@@ -1,4 +1,4 @@
-//===- support.cu - NVPTX OpenMP support functions --- CUDA 
-*-===//
+//===- support.cu - OpenMP support functions - CUDA 
-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
Index: openmp/libomptarget/deviceRTLs/common/src/parallel.cu
===
--- openmp/libomptarget/deviceRTLs/common/src/parallel.cu
+++ openmp/libomptarget/deviceRTLs/common/src/parallel.cu
@@ -1,4 +1,4 @@
-//=== parallel.cu - NVPTX OpenMP parallel implementation - CUDA 
-*-===//
+//=== parallel.cu - GPU OpenMP parallel implementation --- CUDA 
-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.


Index: openmp/libomptarget/deviceRTLs/nvptx/CMakeLists.txt
===
--- openmp/libomptarget/deviceRTLs/nvptx/CMakeLists.txt
+++ openmp/libomptarget/deviceRTLs/nvptx/CMakeLists.txt
@@ -57,10 +57,10 @@
   src/libcall.cu
   ${devicertl_common_directory}/src/loop.cu
   ${devicertl_common_directory}/src/omptarget.cu
-  src/parallel.cu
+  ${devicertl_common_directory}/src/parallel.cu
   src/reduction.cu
-  src/support.cu
-  src/sync.cu
+  ${devicertl_common_directory}/src/support.cu
+  ${devicertl_common_directory}/src/sync.cu
   ${devicertl_common_directory}/src/task.cu
   )
 
Index: openmp/libomptarget/deviceRTLs/common/src/sync.cu
===
--- openmp/libomptarget/deviceRTLs/common/src/sync.cu
+++ openmp/libomptarget/deviceRTLs/common/src/sync.cu
@@ -1,4 +1,4 @@
-//=== sync.h - NVPTX OpenMP synchronizations - CUDA -*-===//
+//=== sync.cu - OpenMP synchronizations -- CUDA -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
Index: openmp/libomptarget/deviceRTLs/common/src/support.cu
===
--- openmp/libomptarget/deviceRTLs/common/src/support.cu
+++ openmp/libomptarget/deviceRTLs/common/src/support.cu
@@ -1,4 +1,4 @@
-//===- support.cu - NVPTX OpenMP support functions --- CUDA -*-===//
+//===- support.cu - OpenMP support functions - CUDA -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
Index: openmp/libomptarget/deviceRTLs/common/src/parallel.cu
===
--- 

[PATCH] D69494: OpenMP: Add helper function for convergent runtime calls

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a reviewer: grokos.
JonChesterfield accepted this revision.
JonChesterfield added a comment.
This revision is now accepted and ready to land.

LGTM.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69494/new/

https://reviews.llvm.org/D69494



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: llvm/include/llvm/IR/OpenMPKinds.def:186
+///{
+
+#ifndef OMP_IDENT_FLAG

jdoerfert wrote:
> JonChesterfield wrote:
> > Meinersbur wrote:
> > > JonChesterfield wrote:
> > > > jdoerfert wrote:
> > > > > Meinersbur wrote:
> > > > > > jdoerfert wrote:
> > > > > > > JonChesterfield wrote:
> > > > > > > > Sharing constants between the compiler and the runtime is an 
> > > > > > > > interesting subproblem. I think the cleanest solution is the 
> > > > > > > > one used by libc, where the compiler generates header files 
> > > > > > > > containing the constants which the runtime library includes.
> > > > > > > I'd prefer not to tackle this right now but get this done first 
> > > > > > > and revisit the issue later. OK?
> > > > > > I don't think this is a good solution. It means that libomp cannot 
> > > > > > built built anymore without also building clang. Moreover, the 
> > > > > > values cannot be changed anyway since it would break the ABI.
> > > > > > 
> > > > > > I'd go the other route: The libomp defines what it's ABI is, the 
> > > > > > compiler generates code for it. 
> > > > > This patch doesn't change what we do, just where. The numbers are 
> > > > > hard coded in clang now. Let's start a discussion on the list and if 
> > > > > we come up with a different scheme we do it after this landed.
> > > > Revisit later sounds good.
> > > > 
> > > > @Meinersbur Do you know of an example of a non-llvm compiler using this 
> > > > libomp?
> > > > 
> > > > The usual order is build a compiler, then use it to build the runtime 
> > > > libraries, then the whole package can build other stuff. Provided the 
> > > > compiler doesn't need any of the runtime libraries (compiler-rt, maths 
> > > > libraries, libomp etc) itself the system bootstraps cleanly. Especially 
> > > > important when cross compiling and I suspect the gpu targets in openmp 
> > > > have similarly strict requirements on the first compiler.
> > > > 
> > > > Closely related to that, I tend to assume that the runtime libraries 
> > > > can be rewritten to best serve their only client - the associated 
> > > > compiler - so if libomp is used by out of tree compilers I'd like to 
> > > > know who we are at risk of breaking.
> > > > Do you know of an example of a non-llvm compiler using this libomp?
> > > 
> > > [[ 
> > > https://github.com/llvm-project/llvm-project/blob/master/openmp/runtime/src/kmp_gsupport.cpp
> > >  | gcc  ]] (using libomp's gomp compatibility layer), [[ 
> > > https://www.openmprtl.org/ | icc  ]] (as libomp was initially donated by 
> > > Intel).
> > > 
> > > I don't understand why it even matters if there are other compilers using 
> > > libomp. Every LLVM runtime library can be built stand-alone. 
> > > With constant values being determined during compiler bootstrapping, 
> > > programs built on one computer would be potentially ABI-incompatible with 
> > > a runtime library on another. Think about updating your 
> > > compiler-rt/libomp/libc++ on you computer causing all existing binaries 
> > > on the system to crash because constants changed in the updated 
> > > compiler's bootstrapping process.
> > > 
> > > The only use case I know that does this is are operating system's syscall 
> > > tables. Linux's reference is [[ 
> > > https://github.com/torvalds/linux/blob/master/arch/sh/include/uapi/asm/unistd_64.h
> > >  | unistd.h ]] which is platform-specific and Windows generates the table 
> > > during its [[ https://j00ru.vexillium.org/syscalls/nt/64/ | build process 
> > > ]]. Therefore on Windows, system calls can only be done through ntdll. 
> > > Even on Linux one should use the system's libc instead of directly 
> > > invoking a system call.
> > Thanks. GCC and ICC would presumably be happier with the magic numbers 
> > stored with openmp then (though with the move to a monorepo that's a little 
> > less persuasive).
> > 
> > When constants that affect the ABI change, the result won't work with 
> > existing software regardless of whether the compiler or the library 
> > contains the change. Either the new compiler builds things that don't work 
> > with the old library, or the new library doesn't work with things built by 
> > the old compiler. The two have to agree on the ABI.
> > 
> > At present, openmp does the moral equivalent of #include OpenMPKinds.def 
> > from clang. Moving the constants to libomp means clang will do the 
> > equivalent of #include OpenMPKinds.def from openmp. Breaking that 
> > dependency means making a new subproject that just holds/generates the 
> > constants, that both depend on, which seems more hassle than it's worth. 
> > 
> > I'd like to generate this header as part of the clang build (though 
> > ultimately don't care that much if it's generated as part of the openmp 
> > build) because it's going to become increasingly challenging to read as 
> > non-nvptx architectures are introduced. Likewise it 

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield accepted this revision.
JonChesterfield added a comment.
This revision is now accepted and ready to land.

In D69785#1763317 , @jdoerfert wrote:

> I'm confused. Was this a review? I'm waiting for a decision here so we can 
> move on and improve on this instead of me modifying it inp-lace two comments 
> at a time.


Explicitly marked as accepted. Patch has looked good for a while and even has 
other people building on it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69785/new/

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

I'd very much like this to land soon. It's the prereq for a lot of other 
patches and the code looks good.

It's tricky to test the infra before the users are landed so the unit test is 
particularly appreciated.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69785/new/

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

In D64943#1682452 , @hfinkel wrote:

> This LGTM. I'm happy that this is a design improvement over the current 
> scheme. @JonChesterfield , @ABataev , any further comments?


This patch mixes two concerns. 
1/ Remove the linker script
2/ Change generation of offload registration code

These should be separate patches. I think the linker script removal would then 
be uncontentious.

It'll be easier to consider the offload registration changes without the linker 
script changes. That's a more complicated design space. In particular, this 
change is motivated by supporting additional platforms, and I don't see how 
offload registration is related to that.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64943/new/

https://reviews.llvm.org/D64943



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

The three way split looks great, thanks.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64943/new/

https://reviews.llvm.org/D64943



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68166: [Clang][OpenMP Offload] Add new tool for wrapping offload device binaries

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield accepted this revision.
JonChesterfield added a comment.
This revision is now accepted and ready to land.

The direction is good and I believe all the feedback from D64943 
 has already been incorporated. LGTM, thanks.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68166/new/

https://reviews.llvm.org/D68166



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68166: [Clang][OpenMP Offload] Add new tool for wrapping offload device binaries

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

> The tool indeed does not have anything specific to OpenMP at this step, but 
> that will change...

That makes sense to me, thanks.

I think we're going to have some trouble adapting this to our build as there's 
already a standalone tool that runs at link time. Overall dropping the linker 
script is probably worth the integration headache.




Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:84
+  void createImages(ArrayRef Binaries) {
+for (const BinaryDesc  : Binaries) {
+  StringRef SectionName = SS.save(".omp_offloading." + Bin.second);

sdmitriev wrote:
> JonChesterfield wrote:
> > I don't think this works for multiple binaries with the same target triple. 
> > They'll all be put in the same section and there will be duplicate symbols 
> > for start/end.
> Adding the same target triple to the list of OpenMP targets more than once is 
> not supported, so such use case isn't viable:
> 
> ```
> bash-4.2$ clang -fopenmp 
> -fopenmp-targets=x86_64-pc-linux-gnu,x86_64-pc-linux-gnu test.c
> clang-10: warning: The OpenMP offloading target 'x86_64-pc-linux-gnu' is 
> similar to target 'x86_64-pc-linux-gnu' already specified - will be ignored. 
> [-Wopenmp-target]
> bash-4.2$ 
> ```
> 
> But in any case I am going to remove the code which passes offload target 
> triples to the wrapper tool in the last part of D64943 because they will not 
> be needed for creating wrapper bit-code. As you know start/end symbols are 
> referenced from the offload registration code only, so, moving offload 
> registration code to the wrapper bit-code eliminates the need to create 
> global start/end symbols with predefined names derived from the triple.
That's true. It seems a shame that we can embed at most one device binary per 
architecture into the host, but that's an existing limitation.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68166/new/

https://reviews.llvm.org/D68166



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71830: [OpenMP] Reusable OpenMP context/traits handling

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

Big patch but looks like a net decrease in complexity. Please could you clang 
format the areas phabricator is complaining about?

Reading through on a browser looks great. I'll take a closer look in a real 
editor once Christmas is out of the way. Thanks for posting this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71830/new/

https://reviews.llvm.org/D71830



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74361: [Clang] Uninitialize attribute on global variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield created this revision.
JonChesterfield added reviewers: kcc, rjmccall, rsmith, glider, vitalybuka, 
pcc, eugenis, vlad.tsyrklevich, jdoerfert, gregrodgers.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

[Clang] Uninitialize attribute on global variables

Extends D54604  to permit [[uninitialized]] on 
global and static variables

Initializing global variables is very cheap on hosted implementations. The
C semantics of zero initializing globals work very well there. It is not
necessarily cheap on freestanding implementations. Where there is no loader
available, code must be emitted near the start point to write the appropriate
values into memory.

At present, external variables can be declared in C++ and definitions provided
in assembly (or IR) to achive this effect. This patch removes a restriction on
the existing attribute in order to remove this reason for writing assembly for
performance sensitive freestanding implementations.

A close analogue in tree is LDS memory for amdgcn, where the kernel is
responsible for initializing the memory after it starts executing on the gpu.
Uninitalized variables in LDS are observably cheaper than zero initialized.

Patch follows the cuda __shared__ variable implementation which also produces
undef global variables, and reuses the [[uninitialized]] attribute from auto
variable initialisation. I think the existing docs are still appropriate.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D74361

Files:
  clang/include/clang/Basic/Attr.td
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGenCXX/attribute_uninitialized.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-uninitialized.c

Index: clang/test/Sema/attr-uninitialized.c
===
--- clang/test/Sema/attr-uninitialized.c
+++ clang/test/Sema/attr-uninitialized.c
@@ -6,16 +6,11 @@
 
 void bad() {
   int im_bad __attribute((uninitialized("zero")));  // expected-error {{'uninitialized' attribute takes no arguments}}
-  static int im_baaad __attribute((uninitialized)); // expected-warning {{'uninitialized' attribute only applies to local variables}}
 }
 
-extern int come_on __attribute((uninitialized));// expected-warning {{'uninitialized' attribute only applies to local variables}}
-int you_know __attribute((uninitialized));  // expected-warning {{'uninitialized' attribute only applies to local variables}}
-static int and_the_whole_world_has_to __attribute((uninitialized)); // expected-warning {{'uninitialized' attribute only applies to local variables}}
-
-void answer_right_now() __attribute((uninitialized)) {}// expected-warning {{'uninitialized' attribute only applies to local variables}}
-void just_to_tell_you_once_again(__attribute((uninitialized)) int whos_bad) {} // expected-warning {{'uninitialized' attribute only applies to local variables}}
+void answer_right_now() __attribute((uninitialized)) {}// expected-warning {{'uninitialized' attribute only applies to variables}}
+void just_to_tell_you_once_again(__attribute((uninitialized)) int whos_bad) {}
 
 struct TheWordIsOut {
-  __attribute((uninitialized)) int youre_doin_wrong; // expected-warning {{'uninitialized' attribute only applies to local variables}}
-} __attribute((uninitialized));  // expected-warning {{'uninitialized' attribute only applies to local variables}}
+  __attribute((uninitialized)) int youre_doin_wrong; // expected-warning {{'uninitialized' attribute only applies to variables}}
+} __attribute((uninitialized));  // expected-warning {{'uninitialized' attribute only applies to variables}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -148,6 +148,7 @@
 // CHECK-NEXT: Target (SubjectMatchRule_function)
 // CHECK-NEXT: TestTypestate (SubjectMatchRule_function_is_member)
 // CHECK-NEXT: TrivialABI (SubjectMatchRule_record)
+// CHECK-NEXT: Uninitialized (SubjectMatchRule_variable)
 // CHECK-NEXT: UseHandle (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: VecReturn (SubjectMatchRule_record)
 // CHECK-NEXT: VecTypeHint (SubjectMatchRule_function)
Index: clang/test/CodeGenCXX/attribute_uninitialized.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/attribute_uninitialized.cpp
@@ -0,0 +1,40 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -std=c++11 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: @_ZZ4funcvE4data = internal global i32 undef
+int* func(void)
+{
+  

[PATCH] D74513: [OpenMP][NFCI] Use the libFrontend DefaultKind in Clang

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

Nice. Thank you


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74513/new/

https://reviews.llvm.org/D74513



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

Procedural note - adding someone as a blocking reviewer to someone else's patch 
isn't great. What if the new reviewer never gets around to looking at the patch?

I've adjusted that to non-blocking, but feel free to leave a comment if I've 
missed something.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71830/new/

https://reviews.llvm.org/D71830



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74571: [OpenMP][CUDA] Add CUDA 10.2 support

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

Do the in tree tests all pass with the 10.2 toolchain? That's not exactly the 
same as whether it works but is the closest approximation we have available.

Assuming yes, this patch seems uncontroversial.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74571/new/

https://reviews.llvm.org/D74571



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59319: [OpenMP][Offloading][1/3] A generic and simple target region interface

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

I'd like to rebase this on the current deviceRTL, and add any nvptx/amdgcn 
specific hooks if necessary. Any objections?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59319/new/

https://reviews.llvm.org/D59319



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74925: [OpenMP][Opt] Combine `struct ident_t*` during deduplication

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield accepted this revision.
JonChesterfield added a comment.
This revision is now accepted and ready to land.

Nice. Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74925/new/

https://reviews.llvm.org/D74925



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74361: [Clang] Undef attribute for global variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

The above patch composes sensibly with openmp, e.g.

  #include 
  #pragma omp declare target
  int data __attribute__((no_zero_initializer));
  #pragma omp allocate(data) allocator(omp_pteam_mem_alloc)
  #pragma omp end declare target

  @data = hidden addrspace(3) global i32 undef, align 4

I like `loader_uninitialized`. There's some prior art on using NOLOAD from a 
(gnu) linker script to achieve the same result, which is a similar name. I'll 
update the patch accordingly.

I found an arm toolchain which supports UNINIT in linker scripts. Asking around 
I've also heard that games dev is a potential user for this feature (perhaps 
analogous to mmap's MAP_UNINITIALIZED?) but haven't been able to confirm 
specifics.

I'll take a first shot at the documentation too.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74361/new/

https://reviews.llvm.org/D74361



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74361: [Clang] Undef attribute for global variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield updated this revision to Diff 247617.
JonChesterfield added a comment.

- Rename attribute, propose some documentation


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74361/new/

https://reviews.llvm.org/D74361

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/AST/DeclBase.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-loader-uninitialized.cpp

Index: clang/test/Sema/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.cpp
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only
+
+int good __attribute__((loader_uninitialized));
+const int still_cant_be_const __attribute__((loader_uninitialized)); // expected-error {{default initialization of an object of const type}}
+extern int external __attribute__((loader_uninitialized));
+
+void func() __attribute__((loader_uninitialized)) // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
+{
+  int local __attribute__((loader_uninitialized)); // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static int sl __attribute__((loader_uninitialized));
+}
+
+struct s {
+  __attribute__((loader_uninitialized)) int field; // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static __attribute__((loader_uninitialized)) int sfield;
+
+} __attribute__((loader_uninitialized)); // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -65,6 +65,7 @@
 // CHECK-NEXT: InitPriority (SubjectMatchRule_variable)
 // CHECK-NEXT: InternalLinkage (SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_record)
 // CHECK-NEXT: LTOVisibilityPublic (SubjectMatchRule_record)
+// CHECK-NEXT: LoaderUninitialized (SubjectMatchRule_variable_is_global)
 // CHECK-NEXT: Lockable (SubjectMatchRule_record)
 // CHECK-NEXT: MIGServerRoutine (SubjectMatchRule_function, SubjectMatchRule_objc_method, SubjectMatchRule_block)
 // CHECK-NEXT: MSStruct (SubjectMatchRule_record)
Index: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
@@ -0,0 +1,40 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -std=c++11 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: @_ZZ4funcvE4data = internal global i32 undef
+int* func(void)
+{
+  static int data [[clang::loader_uninitialized]];
+  return 
+}
+
+// No code emitted
+extern int extern_unhelpful_but_harmless [[clang::loader_uninitialized]];
+
+// CHECK: @tentative = global i32 undef
+int tentative  [[clang::loader_uninitialized]];
+
+// CHECK: @_ZL16tentative_static = internal global i32 undef
+static int tentative_static [[clang::loader_uninitialized]] __attribute__((used));
+
+// CHECK: @nominally_zero_init = global i32 undef
+int nominally_zero_init  [[clang::loader_uninitialized]] = 0;
+
+// CHECK: @nominally_value_init = global i32 undef
+int nominally_value_init  [[clang::loader_uninitialized]] = 4;
+
+class trivial
+{
+  float x;
+};
+
+// CHECK: @ut = global %class.trivial undef
+trivial ut [[clang::loader_uninitialized]];
+
+struct nontrivial
+{
+  nontrivial() : x(3.14) {}
+  double x;
+};
+
+// CHECK: @unt = global %struct.nontrivial undef
+nontrivial unt [[clang::loader_uninitialized]];
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6505,6 +6505,11 @@
   D->addAttr(::new (S.Context) UninitializedAttr(S.Context, AL));
 }
 
+static void handleLoaderUninitializedAttr(Sema , Decl *D,
+const ParsedAttr ) {
+  D->addAttr(::new (S.Context) LoaderUninitializedAttr(S.Context, AL));
+}
+
 static bool tryMakeVariablePseudoStrong(Sema , VarDecl *VD,
 bool DiagnoseFailure) {
   QualType Ty = VD->getType();
@@ -7427,6 +7432,10 @@
 handleUninitializedAttr(S, D, AL);
 break;
 
+  case ParsedAttr::AT_LoaderUninitialized:
+handleLoaderUninitializedAttr(S, D, AL);
+break;
+
   case ParsedAttr::AT_ObjCExternallyRetained:
 handleObjCExternallyRetainedAttr(S, D, AL);
 break;
Index: clang/lib/CodeGen/CodeGenModule.cpp

[PATCH] D74361: [Clang] Undef attribute for global variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield updated this revision to Diff 247652.
JonChesterfield marked 2 inline comments as done.
JonChesterfield added a comment.

- Address review comments, more test coverage


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74361/new/

https://reviews.llvm.org/D74361

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/AST/DeclBase.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-loader-uninitialized.c
  clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-loader-uninitialized.cpp

Index: clang/test/Sema/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.cpp
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only
+
+int good __attribute__((loader_uninitialized));
+const int still_cant_be_const __attribute__((loader_uninitialized)); // expected-error {{default initialization of an object of const type}}
+extern int external __attribute__((loader_uninitialized));
+
+int noargs __attribute__((loader_uninitialized(0))); // expected-error {{'loader_uninitialized' attribute takes no arguments}} 
+
+void func() __attribute__((loader_uninitialized)) // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
+{
+  int local __attribute__((loader_uninitialized)); // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static int sl __attribute__((loader_uninitialized));
+}
+
+struct s {
+  __attribute__((loader_uninitialized)) int field; // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static __attribute__((loader_uninitialized)) int sfield;
+
+} __attribute__((loader_uninitialized)); // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
+
+int redef_attr_first __attribute__((loader_uninitialized));
+int redef_attr_first;
+// expected-error@-1 {{redefinition of 'redef_attr_first'}}
+// expected-note@-3 {{previous definition is here}}
+
+int redef_attr_second; 
+int redef_attr_second __attribute__((loader_uninitialized)); 
+// expected-warning@-1 {{attribute declaration must precede definition}}
+// expected-note@-3 {{previous definition is here}}
+// expected-error@-3 {{redefinition of 'redef_attr_second'}}
+// expected-note@-5 {{previous definition is here}}
+
+// Would like sema to reject this, but that is not yet implemented
+int multiple_definitions __attribute__((loader_uninitialized)) = 42;
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -65,6 +65,7 @@
 // CHECK-NEXT: InitPriority (SubjectMatchRule_variable)
 // CHECK-NEXT: InternalLinkage (SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_record)
 // CHECK-NEXT: LTOVisibilityPublic (SubjectMatchRule_record)
+// CHECK-NEXT: LoaderUninitialized (SubjectMatchRule_variable_is_global)
 // CHECK-NEXT: Lockable (SubjectMatchRule_record)
 // CHECK-NEXT: MIGServerRoutine (SubjectMatchRule_function, SubjectMatchRule_objc_method, SubjectMatchRule_block)
 // CHECK-NEXT: MSStruct (SubjectMatchRule_record)
Index: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -std=c++11 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: @_ZZ4funcvE4data = internal global i32 undef
+int* func(void)
+{
+  static int data [[clang::loader_uninitialized]];
+  return 
+}
+
+// No code emitted
+extern int extern_unhelpful_but_harmless [[clang::loader_uninitialized]];
+
+// CHECK: @defn = global i32 undef
+int defn  [[clang::loader_uninitialized]];
+
+// CHECK: @_ZL11defn_static = internal global i32 undef
+static int defn_static [[clang::loader_uninitialized]] __attribute__((used));
+
+// CHECK: @nominally_zero_init = global i32 undef
+int nominally_zero_init  [[clang::loader_uninitialized]] = 0;
+
+// CHECK: @nominally_value_init = global i32 undef
+int nominally_value_init  [[clang::loader_uninitialized]] = 4;
+
+class trivial
+{
+  float x;
+};
+
+// CHECK: @ut = global %class.trivial undef
+trivial ut [[clang::loader_uninitialized]];
+
+struct nontrivial
+{
+  nontrivial() : x(3.14) {}
+  double x;
+};
+
+// CHECK: @unt = global %struct.nontrivial undef
+nontrivial unt [[clang::loader_uninitialized]];
+
+// CHECK: @arr = global [32 x double] undef, align 16
+double arr[32] __attribute__((loader_uninitialized));
+
+// 

[PATCH] D74361: [Clang] Undef attribute for global variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield marked 2 inline comments as done.
JonChesterfield added a comment.

Fixed the spelling/formatting, added more tests. The C++ case would be improved 
by warning on `int x __attribute__((loader_uninitialised)) = 0` as there are 
two initializers.

The semantics for C are not what I hoped for where there are multiple 
definitions, one of which is via this attribute. Added a test for that. 
Recommendations for where to poke sema to raise an error on the second one are 
warmly invited.




Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7436
+  case ParsedAttr::AT_LoaderUninitialized:
+handleLoaderUninitializedAttr(S, D, AL);
+break;

aaron.ballman wrote:
> If you don't need any custom semantic checking, you can remove that function 
> and instead call `handleSimpleAttribute(S, D, AL);`
I think this patch does need some custom semantic checking, I just haven't been 
able to work out how to implement it. Specifically, the attribute is an 
initializer, so

`int foo __attribute__((loader_uninitialised)) = some_value;`

should be a warning, as the = some_value is going to be ignored.



Comment at: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp:14
+// CHECK: @tentative = global i32 undef
+int tentative  [[clang::loader_uninitialized]];
+

aaron.ballman wrote:
> What should happen with redeclarations? e.g., in C:
> ```
> int a;
> 
> int foo() { return a; }
> 
> int a __attribute__((loader_uninitialized));
> ```
> (This would be a useful test case to add.)
> 
> Also, I'd like to see a test case where the attributed global is an array.
Ah, yes. I was thinking about tentative definitions before changing this test 
to C++. Fixed the name to be less misleading.

C++ just rejects it. Multiple definitions => error. Added test to sema.

C accepts it in either order. Which I believe it should. Either one is a 
tentative definition, and the other provides an actual definition (of undef), 
or the definition (of undef) is followed by a redeclaration.

This leaves the hole that while the following is rightly rejected in C for 
having multiple definitions:
```int a __attr__(...);
int a = 10;```

This is erroneously accepted, with the attribute ignored:
```int a = 10;
int a __attr__(...);```






Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74361/new/

https://reviews.llvm.org/D74361



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74361: [Clang] Undef attribute for global variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

I've continued thinking about / experimenting with this. Curiously - `X x;` and 
`X x{};` are considered distinct by clang. The current patch will only accept 
the former. I'll add some tests for that.

I think there's a reasonable chance that the developers who want to elide the 
runtime cost of zero initialising will usually also want to avoid dynamic 
initialisation. That suggests we could accept only trivially default 
constructible classes, where the undef initializer is correct in that one 
cannot determine whether a no-op constructor actually ran or not. If we go with 
that, then the more complicated question of exactly how this should interact 
with user-controlled disabling of dynamic initialization can be postponed until 
that feature is introduced. This patch is then reasonably self contained.

This patch is strongly related to the linker script approach, or equivalent 
asm. It's target-dependent how the uninitialised data gets represented, e.g. 
x64 will put it in bss anyway. Opencl's __local is similarly uninitialised, and 
gets annotated with an address space that maps onto magic in llc and/or the 
loader.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74361/new/

https://reviews.llvm.org/D74361



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74361: [Clang] Undef attribute for global variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield updated this revision to Diff 247745.
JonChesterfield marked 2 inline comments as done.
JonChesterfield added a comment.

- Reduce scope to trivial default constructed types


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74361/new/

https://reviews.llvm.org/D74361

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/DeclBase.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-loader-uninitialized.c
  clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-loader-uninitialized.cpp

Index: clang/test/Sema/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.cpp
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only
+
+int good __attribute__((loader_uninitialized));
+
+const int still_cant_be_const __attribute__((loader_uninitialized)); // expected-error {{default initialization of an object of const type}}
+extern int external_should_be_rejected __attribute__((loader_uninitialized));
+
+int noargs __attribute__((loader_uninitialized(0))); // expected-error {{'loader_uninitialized' attribute takes no arguments}} 
+
+void func() __attribute__((loader_uninitialized)) // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
+{
+  int local __attribute__((loader_uninitialized)); // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static int sl __attribute__((loader_uninitialized));
+}
+
+struct s {
+  __attribute__((loader_uninitialized)) int field; // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static __attribute__((loader_uninitialized)) int sfield;
+
+} __attribute__((loader_uninitialized)); // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
+
+int redef_attr_first __attribute__((loader_uninitialized));
+int redef_attr_first;
+// expected-error@-1 {{redefinition of 'redef_attr_first'}}
+// expected-note@-3 {{previous definition is here}}
+
+int redef_attr_second; 
+int redef_attr_second __attribute__((loader_uninitialized)); 
+// expected-warning@-1 {{attribute declaration must precede definition}}
+// expected-note@-3 {{previous definition is here}}
+// expected-error@-3 {{redefinition of 'redef_attr_second'}}
+// expected-note@-5 {{previous definition is here}}
+
+int init_rejected __attribute__((loader_uninitialized)) = 42;
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute cannot have an initializer}}
+
+struct trivial {};
+
+trivial default_ok __attribute__((loader_uninitialized));
+trivial value_rejected  __attribute__((loader_uninitialized)) {};
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute cannot have an initializer}}
+
+struct nontrivial
+{
+  nontrivial() {}
+};
+
+nontrivial needs_trivial_ctor __attribute__((loader_uninitialized));
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute must have a trivial default constructor}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -65,6 +65,7 @@
 // CHECK-NEXT: InitPriority (SubjectMatchRule_variable)
 // CHECK-NEXT: InternalLinkage (SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_record)
 // CHECK-NEXT: LTOVisibilityPublic (SubjectMatchRule_record)
+// CHECK-NEXT: LoaderUninitialized (SubjectMatchRule_variable_is_global)
 // CHECK-NEXT: Lockable (SubjectMatchRule_record)
 // CHECK-NEXT: MIGServerRoutine (SubjectMatchRule_function, SubjectMatchRule_objc_method, SubjectMatchRule_block)
 // CHECK-NEXT: MSStruct (SubjectMatchRule_record)
Index: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -std=c++11 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: @_ZZ4funcvE4data = internal global i32 undef
+int* func(void)
+{
+  static int data [[clang::loader_uninitialized]];
+  return 
+}
+
+// No code emitted
+extern int extern_unhelpful_but_harmless [[clang::loader_uninitialized]];
+
+// CHECK: @defn = global i32 undef
+int defn  [[clang::loader_uninitialized]];
+
+// CHECK: @_ZL11defn_static = internal global i32 undef
+static int defn_static [[clang::loader_uninitialized]] __attribute__((used));
+
+class trivial
+{
+  float x;
+};

[PATCH] D74361: [Clang] Undef attribute for global variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield marked 4 inline comments as done.
JonChesterfield added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:4367
+This is useful for variables that are always written to before use where the
+default zero initialization provided by the toolchain loader is expensive.
+  }];

rjmccall wrote:
> How about:
> 
> > The ``loader_uninitialized`` attribute can be placed on global variables to
> > indicate that the variable does not need to be zero initialized by the 
> > loader.
> > On most targets, zero-initialization does not incur any additional cost.
> > For example, most general purpose operating systems deliberately ensure
> > that all memory is properly initialized in order to avoid leaking privileged
> > information from the kernel or other programs.  However, some targets
> > do not make this guarantee, and on these targets, avoiding an unnecessary
> > zero-initialization can have a significant impact on load times and/or code
> > size.
> >
> > A declaration with this attribute is a non-tentative definition just as if 
> > it
> > provided an initializer.   Variables with this attribute are considered to 
> > be
> > uninitialized in the same sense as a local variable, and the programs must
> > write to them before reading from them.  If the variable's type is a C++ 
> > class
> > type with a non-trivial default constructor, or an array thereof, this 
> > attribute
> > only suppresses the static zero-initialization of the variable, not the 
> > dynamic
> > initialization provided by executing the default constructor.
That's a lot better! Thank you. Updated the patch to use your wording verbatim.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7436
+  case ParsedAttr::AT_LoaderUninitialized:
+handleLoaderUninitializedAttr(S, D, AL);
+break;

aaron.ballman wrote:
> rjmccall wrote:
> > JonChesterfield wrote:
> > > aaron.ballman wrote:
> > > > If you don't need any custom semantic checking, you can remove that 
> > > > function and instead call 
> > > > `handleSimpleAttribute(S, D, AL);`
> > > I think this patch does need some custom semantic checking, I just 
> > > haven't been able to work out how to implement it. Specifically, the 
> > > attribute is an initializer, so
> > > 
> > > `int foo __attribute__((loader_uninitialised)) = some_value;`
> > > 
> > > should be a warning, as the = some_value is going to be ignored.
> > This should be an error, not a warning, unless there's a specific need to 
> > be lenient.
> Agreed that this should be an error rather than a warning.
> 
> Not 100% certain, but I suspect you'll need to add that checking to 
> `Sema::AddInitializerToDecl()` because I'm guessing that the initializer has 
> not been processed by the time the attributes are being applied to the 
> variable declaration. You can check `VarDecl::hasInit()` within 
> `handleLoaderUninitializedAttr()` to see if that specific declaration has an 
> initializer, but I'm betting that gives you the wrong answer.
Nice, Yes, that's much better - I should have searched for the opencl `__local` 
handling. As you suspected, hasInit() just returns true at that point.



Comment at: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp:23
+// CHECK: @nominally_value_init = global i32 undef
+int nominally_value_init  [[clang::loader_uninitialized]] = 4;
+

Quuxplusone wrote:
> This test case is identical to line 36 of 
> clang/test/Sema/attr-loader-uninitialized.cpp, where you say you don't want 
> it to compile at all.
> 
> I think you need a clearer idea of how this interacts with initializers. Is 
> it merely supposed to eliminate the //zero-initialization// that happens 
> before the user-specified construction/initialization, or is it supposed to 
> compete with the user-specified construction/initialization?
> 
> That is, for
> 
> nontrivial unt [[clang::loader_uninitialized]];
> 
> is it merely supposed to call `unt::unt()` on a chunk of undef memory 
> (instead of the usual chunk of zeroed memory), or is it supposed to skip the 
> constructor entirely? And for
> 
> int x [[clang::loader_uninitialized]] = foo();
> 
> is it merely supposed to call `foo()` and assign the result to a chunk of 
> undef memory (instead of the usual chunk of zeroed memory), or is it supposed 
> to skip the initialization entirely?
I think you commented while the first working piece of sema landed. My thinking 
is relatively clear but my understanding of clang's semantic analysis is a work 
in progress!

Initializers (`= foo()`) are straightforward. Error on the basis that the 
attribute effectively means `= undef`, and one should not have two 
initializers. A test case is now added for that (and now passes).

The codegen I want for a default constructed global marked with this variable 
is:
- global variable allocated, with undef as the original value
- default constructor call synthesized
- said 

[PATCH] D74361: [Clang] Undef attribute for global variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield updated this revision to Diff 247156.
JonChesterfield added a comment.

- clang-format


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74361/new/

https://reviews.llvm.org/D74361

Files:
  clang/include/clang/Basic/Attr.td
  clang/lib/AST/DeclBase.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGenCXX/attr-no-zero-initializer.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-no-zero-initializer.cpp

Index: clang/test/Sema/attr-no-zero-initializer.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-no-zero-initializer.cpp
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only
+
+int good __attribute__((no_zero_initializer));
+const int still_cant_be_const __attribute__((no_zero_initializer)); // expected-error {{default initialization of an object of const type}}
+extern int external __attribute__((no_zero_initializer));
+
+void func() __attribute__((no_zero_initializer)) // expected-warning {{'no_zero_initializer' attribute only applies to global variables}}
+{
+  int local __attribute__((no_zero_initializer)); // expected-warning {{'no_zero_initializer' attribute only applies to global variables}}
+
+  static int sl __attribute__((no_zero_initializer));
+}
+
+struct s {
+  __attribute__((no_zero_initializer)) int field; // expected-warning {{'no_zero_initializer' attribute only applies to global variables}}
+
+  static __attribute__((no_zero_initializer)) int sfield;
+
+} __attribute__((no_zero_initializer)); // expected-warning {{'no_zero_initializer' attribute only applies to global variables}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -94,6 +94,7 @@
 // CHECK-NEXT: NoStackProtector (SubjectMatchRule_function)
 // CHECK-NEXT: NoThreadSafetyAnalysis (SubjectMatchRule_function)
 // CHECK-NEXT: NoThrow (SubjectMatchRule_hasType_functionType)
+// CHECK-NEXT: NoZeroInitializer (SubjectMatchRule_variable_is_global)
 // CHECK-NEXT: NotTailCalled (SubjectMatchRule_function)
 // CHECK-NEXT: OSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: OSReturnsNotRetained (SubjectMatchRule_function, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property, SubjectMatchRule_variable_is_parameter)
Index: clang/test/CodeGenCXX/attr-no-zero-initializer.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/attr-no-zero-initializer.cpp
@@ -0,0 +1,40 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -std=c++11 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: @_ZZ4funcvE4data = internal global i32 undef
+int* func(void)
+{
+  static int data [[clang::no_zero_initializer]];
+  return 
+}
+
+// No code emitted
+extern int extern_unhelpful_but_harmless [[clang::no_zero_initializer]];
+
+// CHECK: @tentative = global i32 undef
+int tentative  [[clang::no_zero_initializer]];
+
+// CHECK: @_ZL16tentative_static = internal global i32 undef
+static int tentative_static [[clang::no_zero_initializer]] __attribute__((used));
+
+// CHECK: @nominally_zero_init = global i32 undef
+int nominally_zero_init  [[clang::no_zero_initializer]] = 0;
+
+// CHECK: @nominally_value_init = global i32 undef
+int nominally_value_init  [[clang::no_zero_initializer]] = 4;
+
+class trivial
+{
+  float x;
+};
+
+// CHECK: @ut = global %class.trivial undef
+trivial ut [[clang::no_zero_initializer]];
+
+struct nontrivial
+{
+  nontrivial() : x(3.14) {}
+  double x;
+};
+
+// CHECK: @unt = global %struct.nontrivial undef
+nontrivial unt [[clang::no_zero_initializer]];
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6505,6 +6505,11 @@
   D->addAttr(::new (S.Context) UninitializedAttr(S.Context, AL));
 }
 
+static void handleNoZeroInitializerAttr(Sema , Decl *D,
+const ParsedAttr ) {
+  D->addAttr(::new (S.Context) NoZeroInitializerAttr(S.Context, AL));
+}
+
 static bool tryMakeVariablePseudoStrong(Sema , VarDecl *VD,
 bool DiagnoseFailure) {
   QualType Ty = VD->getType();
@@ -7427,6 +7432,10 @@
 handleUninitializedAttr(S, D, AL);
 break;
 
+  case ParsedAttr::AT_NoZeroInitializer:
+handleNoZeroInitializerAttr(S, D, AL);
+break;
+
   case ParsedAttr::AT_ObjCExternallyRetained:
 handleObjCExternallyRetainedAttr(S, D, AL);
 break;
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp

[PATCH] D75285: Mark restrict pointer or reference to const as invariant

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

In D75285#1897247 , @yaxunl wrote:

> If users derive a non-const pointer from the const pointer and modify it, 
> doesn't that result in UB? Thanks.


No. Modifying a const object is UB, so e.g. we can segv if it's in .rodata, but 
a const pointer is not necessarily a pointer to a const object. If it's a const 
pointer to a non-const object then one can cast it directly to a non-const 
pointer and mutate at will.

This unfortunately makes 'const int*' of rather less use than it would 
otherwise be.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75285/new/

https://reviews.llvm.org/D75285



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74361: [Clang] Undef attribute for global variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield marked an inline comment as done.
JonChesterfield added inline comments.



Comment at: clang/test/CodeGenCXX/attr-no-zero-initializer.cpp:40
+// CHECK: @unt = global %struct.nontrivial undef
+nontrivial unt [[clang::no_zero_initializer]];

Quuxplusone wrote:
> Can you explain a bit about how this interacts with C++ constructors? Will 
> this object not have its constructor called at startup; or is it that the 
> constructor will be called but the memory simply won't have been zeroed 
> //before// calling the constructor?
> 
> For [P1144 relocation](https://wg21.link/p1144) (D50114, D61761, etc), Anton 
> Zhilin and I have been discussing the possibility of a similar-sounding 
> attribute that would skip the constructor of a local variable altogether, 
> allowing someone to write e.g.
> ```
> T relocate(T *source) {
> [[unconstructed]] T result;
> memcpy(result, source, sizeof(T));
> return result;
> }
> ```
> If your attribute does exactly that, then I'm interested. If your attribute 
> doesn't do that, but is occupying real estate that //implies// that it does, 
> then I'm also interested.
This change is orthogonal I think. No change to object lifetime. Without the 
attribute, a global is zero initialized and then the constructor is called 
before main. With it, the global is undef before the constructor call.

A different attribute that suppresses the con(de)structor call entirely is also 
of interest to me. A freestanding implementation is likely to accept global 
constructors without warning, and emit code for them, but not actually run them 
at startup. That's not a great UX.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74361/new/

https://reviews.llvm.org/D74361



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74361: [Clang] Undef attribute for global variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74361/new/

https://reviews.llvm.org/D74361



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74941: [OpenMP] `omp begin/end declare variant` - part 1, parsing

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a subscriber: gregrodgers.
JonChesterfield added a comment.

Thanks! Splitting this out of D71179 , which I 
think ultimately reached consensus, makes the diff much easier to read.

Subscribing Greg, as I think this is a path to removing a lot of downstream 
complexity in math.h handling.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74941/new/

https://reviews.llvm.org/D74941



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74361: [Clang] Undef attribute for global variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield updated this revision to Diff 247753.
JonChesterfield added a comment.

- error on extern variables, minor cleanup


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74361/new/

https://reviews.llvm.org/D74361

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/DeclBase.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-loader-uninitialized.c
  clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-loader-uninitialized.cpp

Index: clang/test/Sema/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.cpp
@@ -0,0 +1,58 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only
+
+int good __attribute__((loader_uninitialized));
+
+const int still_cant_be_const __attribute__((loader_uninitialized));
+// expected-error@-1 {{default initialization of an object of const type}}
+extern int external_rejected __attribute__((loader_uninitialized));
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute cannot have external linkage}}
+
+int noargs __attribute__((loader_uninitialized(0)));
+// expected-error@-1 {{'loader_uninitialized' attribute takes no arguments}} 
+
+void func() __attribute__((loader_uninitialized))
+// expected-warning@-1 {{'loader_uninitialized' attribute only applies to global variables}}
+{
+  int local __attribute__((loader_uninitialized));
+  // expected-warning@-1 {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static int sl __attribute__((loader_uninitialized));
+}
+
+struct s {
+  __attribute__((loader_uninitialized)) int field;
+  // expected-warning@-1 {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static __attribute__((loader_uninitialized)) int sfield;
+
+} __attribute__((loader_uninitialized));
+// expected-warning@-1 {{'loader_uninitialized' attribute only applies to global variables}}
+
+int redef_attr_first __attribute__((loader_uninitialized));
+int redef_attr_first;
+// expected-error@-1 {{redefinition of 'redef_attr_first'}}
+// expected-note@-3 {{previous definition is here}}
+
+int redef_attr_second; 
+int redef_attr_second __attribute__((loader_uninitialized)); 
+// expected-warning@-1 {{attribute declaration must precede definition}}
+// expected-note@-3 {{previous definition is here}}
+// expected-error@-3 {{redefinition of 'redef_attr_second'}}
+// expected-note@-5 {{previous definition is here}}
+
+int init_rejected __attribute__((loader_uninitialized)) = 42;
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute cannot have an initializer}}
+
+struct trivial {};
+
+trivial default_ok __attribute__((loader_uninitialized));
+trivial value_rejected  __attribute__((loader_uninitialized)) {};
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute cannot have an initializer}}
+
+struct nontrivial
+{
+  nontrivial() {}
+};
+
+nontrivial needs_trivial_ctor __attribute__((loader_uninitialized));
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute must have a trivial default constructor}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -65,6 +65,7 @@
 // CHECK-NEXT: InitPriority (SubjectMatchRule_variable)
 // CHECK-NEXT: InternalLinkage (SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_record)
 // CHECK-NEXT: LTOVisibilityPublic (SubjectMatchRule_record)
+// CHECK-NEXT: LoaderUninitialized (SubjectMatchRule_variable_is_global)
 // CHECK-NEXT: Lockable (SubjectMatchRule_record)
 // CHECK-NEXT: MIGServerRoutine (SubjectMatchRule_function, SubjectMatchRule_objc_method, SubjectMatchRule_block)
 // CHECK-NEXT: MSStruct (SubjectMatchRule_record)
Index: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: @defn = global i32 undef
+int defn  [[clang::loader_uninitialized]];
+
+// CHECK: @_ZL11defn_static = internal global i32 undef
+static int defn_static [[clang::loader_uninitialized]] __attribute__((used));
+
+// CHECK: @_ZZ4funcvE4data = internal global i32 undef
+int* func(void)
+{
+  static int data [[clang::loader_uninitialized]];
+  return 
+}
+
+class trivial
+{
+  float x;
+};
+
+// CHECK: @ut = global %class.trivial undef
+trivial ut 

[PATCH] D74361: [Clang] Undef attribute for global variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield updated this revision to Diff 247776.
JonChesterfield added a comment.

- Error on redeclaration with loader_uninit in C


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74361/new/

https://reviews.llvm.org/D74361

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/DeclBase.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-loader-uninitialized.c
  clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-loader-uninitialized.c
  clang/test/Sema/attr-loader-uninitialized.cpp

Index: clang/test/Sema/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.cpp
@@ -0,0 +1,58 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only
+
+int good __attribute__((loader_uninitialized));
+
+const int still_cant_be_const __attribute__((loader_uninitialized));
+// expected-error@-1 {{default initialization of an object of const type}}
+extern int external_rejected __attribute__((loader_uninitialized));
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute cannot have external linkage}}
+
+int noargs __attribute__((loader_uninitialized(0)));
+// expected-error@-1 {{'loader_uninitialized' attribute takes no arguments}} 
+
+void func() __attribute__((loader_uninitialized))
+// expected-warning@-1 {{'loader_uninitialized' attribute only applies to global variables}}
+{
+  int local __attribute__((loader_uninitialized));
+  // expected-warning@-1 {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static int sl __attribute__((loader_uninitialized));
+}
+
+struct s {
+  __attribute__((loader_uninitialized)) int field;
+  // expected-warning@-1 {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static __attribute__((loader_uninitialized)) int sfield;
+
+} __attribute__((loader_uninitialized));
+// expected-warning@-1 {{'loader_uninitialized' attribute only applies to global variables}}
+
+int redef_attr_first __attribute__((loader_uninitialized));
+int redef_attr_first;
+// expected-error@-1 {{redefinition of 'redef_attr_first'}}
+// expected-note@-3 {{previous definition is here}}
+
+int redef_attr_second; 
+int redef_attr_second __attribute__((loader_uninitialized)); 
+// expected-warning@-1 {{attribute declaration must precede definition}}
+// expected-note@-3 {{previous definition is here}}
+// expected-error@-3 {{redefinition of 'redef_attr_second'}}
+// expected-note@-5 {{previous definition is here}}
+
+int init_rejected __attribute__((loader_uninitialized)) = 42;
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute cannot have an initializer}}
+
+struct trivial {};
+
+trivial default_ok __attribute__((loader_uninitialized));
+trivial value_rejected  __attribute__((loader_uninitialized)) {};
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute cannot have an initializer}}
+
+struct nontrivial
+{
+  nontrivial() {}
+};
+
+nontrivial needs_trivial_ctor __attribute__((loader_uninitialized));
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute must have a trivial default constructor}}
Index: clang/test/Sema/attr-loader-uninitialized.c
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.c
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only
+// See also attr-loader-uninitialized.cpp
+
+int good __attribute__((loader_uninitialized));
+
+int init_rejected __attribute__((loader_uninitialized)) = 42;
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute cannot have an initializer}}
+
+int declaration_then_uninit_ok;
+int declaration_then_uninit_ok __attribute__((loader_uninitialized));
+
+int definition_then_uninit_rejected = 0;
+int definition_then_uninit_rejected __attribute__((loader_uninitialized));
+// expected-error@-1 {{redeclaration cannot add 'loader_uninitialized' attribute}}
+// expected-note@-3 {{previous definition is here}}
+
+int tentative_repeated_ok __attribute__((loader_uninitialized));
+int tentative_repeated_ok __attribute__((loader_uninitialized));
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -65,6 +65,7 @@
 // CHECK-NEXT: InitPriority (SubjectMatchRule_variable)
 // CHECK-NEXT: InternalLinkage (SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_record)
 // CHECK-NEXT: LTOVisibilityPublic 

[PATCH] D74361: [Clang] Undef attribute for global variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

I'm finally happy with the semantic checks here. Thanks for the guidance on 
where to look for the hooks.

- attributed variable must be at global scope
- all initializers are rejected
- default constructors must be trivial (to reduce the scope of this patch)
- extern variables rejected as they can't meaningfully have a definition
- attribute on a declaration following a normal definition in C rejected

Patch is a bit bigger than I hoped for but quite self contained. Everything is 
guarded by a test on the attribute.

@Quuxplusone does restricting this to trivial default construction resolve your 
concerns?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74361/new/

https://reviews.llvm.org/D74361



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74361: [Clang] Undef attribute for global variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield updated this revision to Diff 247696.
JonChesterfield marked 2 inline comments as done.
JonChesterfield added a comment.

- Reject initialisers, update doc to recommended string


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74361/new/

https://reviews.llvm.org/D74361

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/DeclBase.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-loader-uninitialized.c
  clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-loader-uninitialized.cpp

Index: clang/test/Sema/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.cpp
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only
+
+int good __attribute__((loader_uninitialized));
+
+const int still_cant_be_const __attribute__((loader_uninitialized)); // expected-error {{default initialization of an object of const type}}
+extern int external_should_be_rejected __attribute__((loader_uninitialized));
+
+int noargs __attribute__((loader_uninitialized(0))); // expected-error {{'loader_uninitialized' attribute takes no arguments}} 
+
+void func() __attribute__((loader_uninitialized)) // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
+{
+  int local __attribute__((loader_uninitialized)); // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static int sl __attribute__((loader_uninitialized));
+}
+
+struct s {
+  __attribute__((loader_uninitialized)) int field; // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static __attribute__((loader_uninitialized)) int sfield;
+
+} __attribute__((loader_uninitialized)); // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
+
+int redef_attr_first __attribute__((loader_uninitialized));
+int redef_attr_first;
+// expected-error@-1 {{redefinition of 'redef_attr_first'}}
+// expected-note@-3 {{previous definition is here}}
+
+int redef_attr_second; 
+int redef_attr_second __attribute__((loader_uninitialized)); 
+// expected-warning@-1 {{attribute declaration must precede definition}}
+// expected-note@-3 {{previous definition is here}}
+// expected-error@-3 {{redefinition of 'redef_attr_second'}}
+// expected-note@-5 {{previous definition is here}}
+
+int init_rejected __attribute__((loader_uninitialized)) = 42;
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute cannot have an initializer}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -65,6 +65,7 @@
 // CHECK-NEXT: InitPriority (SubjectMatchRule_variable)
 // CHECK-NEXT: InternalLinkage (SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_record)
 // CHECK-NEXT: LTOVisibilityPublic (SubjectMatchRule_record)
+// CHECK-NEXT: LoaderUninitialized (SubjectMatchRule_variable_is_global)
 // CHECK-NEXT: Lockable (SubjectMatchRule_record)
 // CHECK-NEXT: MIGServerRoutine (SubjectMatchRule_function, SubjectMatchRule_objc_method, SubjectMatchRule_block)
 // CHECK-NEXT: MSStruct (SubjectMatchRule_record)
Index: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
@@ -0,0 +1,41 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -std=c++11 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: @_ZZ4funcvE4data = internal global i32 undef
+int* func(void)
+{
+  static int data [[clang::loader_uninitialized]];
+  return 
+}
+
+// No code emitted
+extern int extern_unhelpful_but_harmless [[clang::loader_uninitialized]];
+
+// CHECK: @defn = global i32 undef
+int defn  [[clang::loader_uninitialized]];
+
+// CHECK: @_ZL11defn_static = internal global i32 undef
+static int defn_static [[clang::loader_uninitialized]] __attribute__((used));
+
+class trivial
+{
+  float x;
+};
+
+// CHECK: @ut = global %class.trivial undef
+trivial ut [[clang::loader_uninitialized]];
+
+struct nontrivial
+{
+  nontrivial() : x(3.14) {}
+  double x;
+};
+
+// CHECK: @unt = global %struct.nontrivial undef
+nontrivial unt [[clang::loader_uninitialized]];
+
+// CHECK: @arr = global [32 x double] undef, align 16
+double arr[32] __attribute__((loader_uninitialized));
+
+// Defining as arr2[] [[clang..]] raises the error: attribute cannot be applied to types
+// CHECK: 

[PATCH] D74361: [Clang] Undef attribute for global variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield marked 2 inline comments as done.
JonChesterfield added inline comments.



Comment at: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp:23
+// CHECK: @nominally_value_init = global i32 undef
+int nominally_value_init  [[clang::loader_uninitialized]] = 4;
+

Quuxplusone wrote:
> rjmccall wrote:
> > JonChesterfield wrote:
> > > Quuxplusone wrote:
> > > > This test case is identical to line 36 of 
> > > > clang/test/Sema/attr-loader-uninitialized.cpp, where you say you don't 
> > > > want it to compile at all.
> > > > 
> > > > I think you need a clearer idea of how this interacts with 
> > > > initializers. Is it merely supposed to eliminate the 
> > > > //zero-initialization// that happens before the user-specified 
> > > > construction/initialization, or is it supposed to compete with the 
> > > > user-specified construction/initialization?
> > > > 
> > > > That is, for
> > > > 
> > > > nontrivial unt [[clang::loader_uninitialized]];
> > > > 
> > > > is it merely supposed to call `unt::unt()` on a chunk of undef memory 
> > > > (instead of the usual chunk of zeroed memory), or is it supposed to 
> > > > skip the constructor entirely? And for
> > > > 
> > > > int x [[clang::loader_uninitialized]] = foo();
> > > > 
> > > > is it merely supposed to call `foo()` and assign the result to a chunk 
> > > > of undef memory (instead of the usual chunk of zeroed memory), or is it 
> > > > supposed to skip the initialization entirely?
> > > I think you commented while the first working piece of sema landed. My 
> > > thinking is relatively clear but my understanding of clang's semantic 
> > > analysis is a work in progress!
> > > 
> > > Initializers (`= foo()`) are straightforward. Error on the basis that the 
> > > attribute effectively means `= undef`, and one should not have two 
> > > initializers. A test case is now added for that (and now passes).
> > > 
> > > The codegen I want for a default constructed global marked with this 
> > > variable is:
> > > - global variable allocated, with undef as the original value
> > > - default constructor call synthesized
> > > - said default constructor set up for invocation from crt, before main, 
> > > writing over the undef value
> > > 
> > > Where the default constructor can be optimized as usual, e.g. if it 
> > > always writes a constant, we can init with that constant instead of the 
> > > undef and elide the constructor.
> > > 
> > > I don't have that actually working yet - the constructor call is not 
> > > being emitted, so we just have the undef global.
> > > 
> > > I think it's important to distinguish between the values of the bits when 
> > > the program is loaded and whether constructor/destructors are called, as 
> > > one could want any combination of the two.
> > I think Arthur is suggesting that it would be useful to allow the attribute 
> > to be used in conjunction with an initializer in C++, since if the 
> > initializer has to be run dynamically, we can still meaningfully suppress 
> > the static zero-initialization.   That is, we've accepted that it's useful 
> > to do this when *default-initializing* a global, but it's actually useful 
> > when doing *any* kind of dynamic initialization.
> > 
> > Maybe we can leave it as an error in C++ when the initializer is a constant 
> > expression.  Although that might be unnecessarily brittle if e.g. the 
> > constructor is `constexpr` in one library version but not another.
> No, that's exctly what I mean. You seem to be holding two contradictory ideas 
> simultaneously:
> 
> [[loader_uninitialized]] X x = X{};  // two initializers, therefore error
> 
> [[loader_uninitialized]] X x {}; // one initializer plus one constructor, 
> therefore fine
> 
> In C++, these two declarations have identical semantics. It doesn't make 
> sense to say that one of them "calls a constructor" and the other one "has an 
> initializer." They're literally the same thing.
> 
> Similarly in both C99 and C++ with plain old ints:
> 
> [[loader_uninitialized]] int x = foo();
> 
> This means "call foo and dynamically initialize x with the result," just as 
> surely as
> 
> [[loader_uninitialized]] X x = X();
> 
> means "call X::X and dynamically initialize x with the result." Having one 
> rule for dynamic initializers of primitive types and a separate rule for 
> dynamic initializers of class types doesn't work.
> 
> Furthermore, "dynamic initialization" can be promoted to compile-time:
> 
> [[loader_uninitialized]] int x = 42;
> [[loader_uninitialized]] std::string_view x = "hello world";
> 
> It wouldn't make semantic sense to say that one of these has "two 
> initializers" and the other has "one initializer," because both of the 
> initializations end up happening at compile time and getting put into .data.
I'm under the impression that the constructs are:
```
X x = X{};  // default construct an X and then call the copy constructor at x
X x {}; // 

[PATCH] D74361: [Clang] Uninitialize attribute on global variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield updated this revision to Diff 247127.
JonChesterfield added a comment.

- Rename attribute, add to hasDefiningAttr


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74361/new/

https://reviews.llvm.org/D74361

Files:
  clang/include/clang/Basic/Attr.td
  clang/lib/AST/DeclBase.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGenCXX/attr-no-zero-initializer.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-no-zero-initializer.cpp

Index: clang/test/Sema/attr-no-zero-initializer.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-no-zero-initializer.cpp
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only
+
+int good __attribute__((no_zero_initializer));
+const int still_cant_be_const __attribute__((no_zero_initializer)); // expected-error {{default initialization of an object of const type}}
+extern int external __attribute__((no_zero_initializer));
+
+void func() __attribute__((no_zero_initializer)) // expected-warning {{'no_zero_initializer' attribute only applies to global variables}}
+{
+  int local __attribute__((no_zero_initializer)); // expected-warning {{'no_zero_initializer' attribute only applies to global variables}}
+
+  static int sl __attribute__((no_zero_initializer));
+}
+
+struct s {
+  __attribute__((no_zero_initializer)) int field; // expected-warning {{'no_zero_initializer' attribute only applies to global variables}}
+
+  static __attribute__((no_zero_initializer)) int sfield;
+
+} __attribute__((no_zero_initializer)); // expected-warning {{'no_zero_initializer' attribute only applies to global variables}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -94,6 +94,7 @@
 // CHECK-NEXT: NoStackProtector (SubjectMatchRule_function)
 // CHECK-NEXT: NoThreadSafetyAnalysis (SubjectMatchRule_function)
 // CHECK-NEXT: NoThrow (SubjectMatchRule_hasType_functionType)
+// CHECK-NEXT: NoZeroInitializer (SubjectMatchRule_variable_is_global)
 // CHECK-NEXT: NotTailCalled (SubjectMatchRule_function)
 // CHECK-NEXT: OSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: OSReturnsNotRetained (SubjectMatchRule_function, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property, SubjectMatchRule_variable_is_parameter)
Index: clang/test/CodeGenCXX/attr-no-zero-initializer.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/attr-no-zero-initializer.cpp
@@ -0,0 +1,40 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -std=c++11 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: @_ZZ4funcvE4data = internal global i32 undef
+int* func(void)
+{
+  static int data [[clang::no_zero_initializer]];
+  return 
+}
+
+// No code emitted
+extern int extern_unhelpful_but_harmless [[clang::no_zero_initializer]];
+
+// CHECK: @tentative = global i32 undef
+int tentative  [[clang::no_zero_initializer]];
+
+// CHECK: @_ZL16tentative_static = internal global i32 undef
+static int tentative_static [[clang::no_zero_initializer]] __attribute__((used));
+
+// CHECK: @nominally_zero_init = global i32 undef
+int nominally_zero_init  [[clang::no_zero_initializer]] = 0;
+
+// CHECK: @nominally_value_init = global i32 undef
+int nominally_value_init  [[clang::no_zero_initializer]] = 4;
+
+class trivial
+{
+  float x;
+};
+
+// CHECK: @ut = global %class.trivial undef
+trivial ut [[clang::no_zero_initializer]];
+
+struct nontrivial
+{
+  nontrivial() : x(3.14) {}
+  double x;
+};
+
+// CHECK: @unt = global %struct.nontrivial undef
+nontrivial unt [[clang::no_zero_initializer]];
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6505,6 +6505,10 @@
   D->addAttr(::new (S.Context) UninitializedAttr(S.Context, AL));
 }
 
+static void handleNoZeroInitializerAttr(Sema , Decl *D, const ParsedAttr ) {
+  D->addAttr(::new (S.Context) NoZeroInitializerAttr(S.Context, AL));
+}
+
 static bool tryMakeVariablePseudoStrong(Sema , VarDecl *VD,
 bool DiagnoseFailure) {
   QualType Ty = VD->getType();
@@ -7427,6 +7431,10 @@
 handleUninitializedAttr(S, D, AL);
 break;
 
+  case ParsedAttr::AT_NoZeroInitializer:
+handleNoZeroInitializerAttr(S, D, AL);
+break;
+
   case ParsedAttr::AT_ObjCExternallyRetained:
 handleObjCExternallyRetainedAttr(S, D, AL);
 break;
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ 

[PATCH] D74361: [Clang] Undef attribute for global variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

In D74361#1879559 , @rjmccall wrote:

> This will need to impact static analysis and the AST, I think.  Local 
> variables can't be redeclared, but global variables can, so disallowing 
> initializers syntactically when the attribute is present doesn't necessarily 
> stop other declarations from defining the variable.  Also, you need to make 
> the attribute mark something as a definition, the same way that e.g. the 
> alias attribute does.  Also, this probably ought to disable the 
> default-initialization of non-trivial types in C++, in case that's not 
> already done.


I would like this to be the case but am having a tough time understanding how 
sema works. VarDecl::hasInit() looked promising but doesn't appear to indicate 
whether there is a syntactic initialiser (e.g. = 10) present. I will continue 
to investigate. Codegen appears to be working fine.

In D74361#1880904 , @jfb wrote:

> The current uninitialized attribute fits the model C and C++ follow for 
> attributes: they have no semantic meaning for a valid program, in that a 
> compiler can just ignore them and (barring undefined behavior) there's no 
> observable effect to the program. This updated semantic changes the behavior 
> of a valid C and C++ program, because the standards specify the value of 
> uninitialized globals and TLS. I'd much rather have a separate attribute, say 
> `no_zero_init`, to explicitly say what this does.


This proposed attribute can similarly be ignored without observable semantic 
effect. Instead of an IR undef variable, we would have an IR zeroinitialized 
variable, which is a legitimate implementation choice for undef. Adding the 
attribute to an existing program will, in general, change its meaning - but 
that's also true of other attributes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74361/new/

https://reviews.llvm.org/D74361



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74361: [Clang] Undef attribute for global variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield marked an inline comment as done.
JonChesterfield added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6509
+static void handleNoZeroInitializerAttr(Sema , Decl *D, const ParsedAttr 
) {
+  D->addAttr(::new (S.Context) NoZeroInitializerAttr(S.Context, AL));
+}

cast(D)->hasInit() seems to always return true here - presumably the 
error needs to be emitted sometime before this


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74361/new/

https://reviews.llvm.org/D74361



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74372: [OpenMP][IRBuilder] Perform finalization (incl. outlining) late

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

Patch looks good with the above nits.

I'm not totally sure about the callback vs running a separate IR pass after the 
finalize() call, but when the callback is this simple it looks fine. I like 
that this preserves the current semantics.




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:275
+  /// Get a handle for a new region that will be outlined later.
+  OutlineInfo () {
+OutlineInfos.push_back({});

Calling getNewOutlineInfo will invalidate the references previously returned. 
That's not a bug in this patch but looks like it'll be easy to get wrong in 
future.

Perhaps the backing store should be a linked list, such that push_back doesn't 
invalidate any existing references?



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:587
+  CallInst *CI = cast(OutlinedFn.user_back());
+  CI->getParent()->setName("omp_parallel");
+  Builder.SetInsertPoint(CI);

s/parallel/outlined?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74372/new/

https://reviews.llvm.org/D74372



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74372: [OpenMP][IRBuilder] Perform finalization (incl. outlining) late

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield accepted this revision.
JonChesterfield added a comment.
This revision is now accepted and ready to land.

Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74372/new/

https://reviews.llvm.org/D74372



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74571: [OpenMP][CUDA] Add CUDA 10.2 support

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

@tra that's great context, thank you very much for writing it up.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74571/new/

https://reviews.llvm.org/D74571



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74571: [OpenMP][CUDA] Add CUDA 10.2 support

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

Interesting distinction.

Should compiling without warning indicate comprehensive support, or merely that 
we ran the tests and they passed?

I assumed the latter on the basis that we probably don't have comprehensive 
support for cuda 10.1 either. No preference either way though.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74571/new/

https://reviews.llvm.org/D74571



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74361: [Clang] Uninitialize attribute on global variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

It zero initialises on x86, but perhaps by coincidence rather than guarantee.

Fair enough regarding reuse of the existing attribute, I'll create a new one.

Thanks for the pointers on additional cases to test for too. I'll return with 
an improved patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74361/new/

https://reviews.llvm.org/D74361



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71948: [OpenMP] Use the OpenMPIRBuilder for `cancel` directives

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield accepted this revision.
JonChesterfield added a comment.
This revision is now accepted and ready to land.

I like this, thanks. Very clear.




Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:241
+  // This seems to be used only once without much change of reuse, could live 
in
+  // OMPKinds.def but seems not necessary.
+  Value *CancelKind = nullptr;

The integer numbers correspond to the `kmp_cancel_kind_t` enum in 
`runtime/src/kmp.h`. The target offloading presently ignores this argument, but 
the host version has a control flow dependency on it.

I think the enum should be shared between the compiler and the runtime, or 
failing that, some test code should include kmp.h and check the numbers still 
match.

This feels like a familiar point - I've just sent an email to openmp-dev to 
discuss whether we can share constants between the two without copy & paste.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71948/new/

https://reviews.llvm.org/D71948



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

In D75917#1916160 , @sameerds wrote:

> how this builtin fits in with the overall scheme of language-specific and 
> target-specific details of an atomic operation. For example, is this meant 
> only for OpenCL? Does it work with CUDA? Or HIP? What is the behaviour for 
> scope in C++?


Identical to the fence instruction. Which is assumed well thought through 
already, given it's an IR instruction.

As far as I can tell, fence composes sensibly with other IR then generates the 
right thing at the back end. So it looks fit for purpose, just not currently 
available from clang.




Comment at: clang/lib/CodeGen/CGBuiltin.cpp:3707
+Value *Scope = EmitScalarExpr(E->getArg(1));
+auto ScopeModel = AtomicScopeModel::create(AtomicScopeModelKind::OpenCL);
+

sameerds wrote:
> The proposed builtin does not claim to be an OpenCL builtin, so it's probably 
> not correct to simply assume the OpenCL model. Should the model be chosen 
> based on the source language specified?
The only values for AtomicScopeModelKind are none and OpenCL.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75917/new/

https://reviews.llvm.org/D75917



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74361: [Clang] Undef attribute for global variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

ping @aaron.ballman - does that look right to you?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74361/new/

https://reviews.llvm.org/D74361



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74361: [Clang] Undef attribute for global variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield updated this revision to Diff 248807.
JonChesterfield marked 2 inline comments as done.
JonChesterfield added a comment.

- Review comments, add tests for private_extern


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74361/new/

https://reviews.llvm.org/D74361

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/DeclBase.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-loader-uninitialized.c
  clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-loader-uninitialized.c
  clang/test/Sema/attr-loader-uninitialized.cpp

Index: clang/test/Sema/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.cpp
@@ -0,0 +1,60 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only
+
+int good __attribute__((loader_uninitialized));
+static int local_ok __attribute__((loader_uninitialized));
+int hidden_ok __attribute__((visibility("hidden"))) __attribute__((loader_uninitialized));
+
+const int still_cant_be_const __attribute__((loader_uninitialized));
+// expected-error@-1 {{default initialization of an object of const type}}
+extern int external_rejected __attribute__((loader_uninitialized));
+// expected-error@-1 {{external declaration of variable cannot have the 'loader_uninitialized' attribute}}
+
+int noargs __attribute__((loader_uninitialized(0)));
+// expected-error@-1 {{'loader_uninitialized' attribute takes no arguments}} 
+
+int init_rejected __attribute__((loader_uninitialized)) = 42;
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute cannot have an initializer}}
+
+void func() __attribute__((loader_uninitialized))
+// expected-warning@-1 {{'loader_uninitialized' attribute only applies to global variables}}
+{
+  int local __attribute__((loader_uninitialized));
+  // expected-warning@-1 {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static int sl __attribute__((loader_uninitialized));
+}
+
+struct s {
+  __attribute__((loader_uninitialized)) int field;
+  // expected-warning@-1 {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static __attribute__((loader_uninitialized)) int sfield;
+
+} __attribute__((loader_uninitialized));
+// expected-warning@-1 {{'loader_uninitialized' attribute only applies to global variables}}
+
+int redef_attr_first __attribute__((loader_uninitialized));
+int redef_attr_first;
+// expected-error@-1 {{redefinition of 'redef_attr_first'}}
+// expected-note@-3 {{previous definition is here}}
+
+int redef_attr_second; 
+int redef_attr_second __attribute__((loader_uninitialized)); 
+// expected-warning@-1 {{attribute declaration must precede definition}}
+// expected-note@-3 {{previous definition is here}}
+// expected-error@-3 {{redefinition of 'redef_attr_second'}}
+// expected-note@-5 {{previous definition is here}}
+
+struct trivial {};
+
+trivial default_ok __attribute__((loader_uninitialized));
+trivial value_rejected  __attribute__((loader_uninitialized)) {};
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute cannot have an initializer}}
+
+struct nontrivial
+{
+  nontrivial() {}
+};
+
+nontrivial needs_trivial_ctor __attribute__((loader_uninitialized));
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute must have a trivial default constructor}}
Index: clang/test/Sema/attr-loader-uninitialized.c
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.c
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only
+// See also attr-loader-uninitialized.cpp
+
+int good __attribute__((loader_uninitialized));
+static int local_ok __attribute__((loader_uninitialized));
+int hidden_ok __attribute__((visibility("hidden"))) __attribute__((loader_uninitialized));
+
+const int can_still_be_const __attribute__((loader_uninitialized));
+
+extern int external_rejected __attribute__((loader_uninitialized));
+// expected-error@-1 {{external declaration of variable cannot have the 'loader_uninitialized' attribute}}
+
+int noargs __attribute__((loader_uninitialized(0)));
+// expected-error@-1 {{'loader_uninitialized' attribute takes no arguments}}
+
+int init_rejected __attribute__((loader_uninitialized)) = 42;
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute cannot have an initializer}}
+
+int declaration_then_uninit_ok;
+int declaration_then_uninit_ok __attribute__((loader_uninitialized));
+
+int definition_then_uninit_rejected = 0;
+int definition_then_uninit_rejected __attribute__((loader_uninitialized));
+// expected-error@-1 {{redeclaration 

[PATCH] D74361: [Clang] Undef attribute for global variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

> I thought err_loader_uninitialized_extern says that the variable cannot have 
> external linkage?

Embarrassing! This was a badly written error message, now fixed to:
`external declaration of variable cannot have the 'loader_uninitialized' 
attribute`

The premise behind this feature is to be semantically identical to:
`type foo = undef;`

The initializer value is orthogonal to the variable linkage and visibility. If 
`= 4` was ok, so should this attribute be.

What is not meaningful is to declare that a variable is defined elsewhere that 
is uninitialized.
That is, `extern int x = 42; // warning: 'extern' variable has an initializer`
Therefore `[[loader_uninitialized]] int x = 42; // also bad`

This patch makes the latter an error, on the basis that it's definitely a 
mistake to provide two initializers for one variable (one 42, one undef).

C++ thinks `const int x;` is an error and C thinks `const int x;` is fine. This 
patch remains consistent with that.




Comment at: clang/lib/Sema/SemaDecl.cpp:12377
+  }
+  if (Var->getStorageClass() == SC_Extern) {
+Diag(Var->getLocation(), diag::err_loader_uninitialized_extern);

aaron.ballman wrote:
> Should this either be calling `VarDecl::hasExternalStorage()` or looking at 
> the linkage of the variable, rather than at the storage class written in the 
> source?
Interesting question, thank you. SC_Extern is right.

hasExternalStorage is true if extern or private_extern. I hadn't seen 
private_extern before, but it appears to be a way to model hidden visibility. 
It accepts a normal initializer, e.g.
`__private_extern__ int private_extern_can_be_initialised = 10;`
therefore should also work with undef.

Added a test for this (which will fail if SC_Extern is replaced with 
hasExternalStorage).

Replying to linkage out of line as it comes up on a few inline comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74361/new/

https://reviews.llvm.org/D74361



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73979: [HIP] Allow non-incomplete array type for extern shared var

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

In D73979#1857736 , @yaxunl wrote:

> BTW this is requested by HIP users, who have similar code for CUDA and HIP. 
> They found it surprised that nvcc allows it but hip-clang does not.


I think I'm one of the HIP users here, but the above change is not what I was 
hoping for.

I'd like:

  __shared__ int x;
  __shared__ int y;
  __device__ void foo()
  {
assert( != );
x = 2 * y;
  }

to compile and behave as it does on cuda, i.e. the 'x' variable gets allocated 
in __shared__ memory for each kernel which accesses it, and so does 'y'.

The 'extern __shared__' feature where nvcc builds a union out of all the things 
it sees and the user indexes into it at runtime is totally unappealing. That 
cuda uses the 'extern' keyword to opt into this magic union also seems 
undesirable.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73979/new/

https://reviews.llvm.org/D73979



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75788: [WIP][OpenMP] Reuse CUDA wrappers in `nvptx` target regions.

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

That's less invasive than I feared. Nicely done.

It may worth keeping the openmp header wrapper to do architecture dispatch. 
Something like:

  #ifndef __CLANG_OPENMP_MATH_DECLARES_H__
  #define __CLANG_OPENMP_MATH_DECLARES_H__
  
  #ifndef _OPENMP
  #error "This file is for OpenMP compilation only."
  #endif
  
  #if defined(__AMDGCN__)
  #pragma omp begin declare variant match(device = {arch(amdgcn)})
  #include "equivalent_header.h"
  #pragma omp end declare variant
  #endif // __AMDGCN__
  
  #if defined(__NVPTX__)
  #define __CUDA__
  #pragma omp begin declare variant match(device = {arch(nvptx)})
  
  #if defined(__cplusplus)
  #include <__clang_cuda_math_forward_declares.h>
  #endif
  
  /// Include declarations for libdevice functions.
  #include <__clang_cuda_libdevice_declares.h>
  /// Provide definitions for these functions.
  #include <__clang_cuda_device_functions.h>
  
  #pragma omp end declare variant
  #undef __CUDA__
  #endif // __NVPTX__
  
  
  #endif // __CLANG_OPENMP_MATH_DECLARES_H__




Comment at: clang/lib/Headers/cuda_wrappers/new:36
 
+#ifdef _OPENMP
+#define __DEVICE__

macros look off here - should it be `#define DEVICE`, or the following uses 
`__DEVICE__`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75788/new/

https://reviews.llvm.org/D75788



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:3713
+  switch (ord) {
+  case 0:  // memory_order_relaxed
+  default: // invalid order

Interesting, fence can't be relaxed

> ‘fence’ instructions take an ordering argument which defines what 
> synchronizes-with edges they add. They can only be given acquire, release, 
> acq_rel, and seq_cst orderings.





Comment at: clang/lib/Sema/SemaChecking.cpp:1880
+// Check if Order is an unsigned
+if (!Ty->isIntegerType()) {
+  Diag(ArgExpr->getExprLoc(), diag::err_typecheck_expect_uint) << Ty;

isIntegerType will return true for signed integers as well as unsigned. It 
seems reasonable to call this with a signed integer type (e.g. '2'), so perhaps 
the references to unsigned should be dropped from the code and error message




Comment at: clang/lib/Sema/SemaChecking.cpp:1889
+
+// Check if Order is one of the valid types
+if (!llvm::isValidAtomicOrderingCABI(ord)) {

This should reject 'relaxed' - I think that's currently accepted by sema then 
silently thrown away by codegen



Comment at: clang/test/CodeGenOpenCL/atomic-ops.cl:291
 
+void test_memory_fence() {
+  // CHECK-LABEL: @test_memory_fence

I'm hoping this intrinsic will be usable from C or C++, as well as from OpenCL 
- please could you add a non-opencl codegen test.

It doesn't need to check all the cases again, just enough to show that the 
intrinsic and arguments are available (they're spelled like `__ATOMIC_SEQ_CST`, 
`__OPENCL_MEMORY_SCOPE_ALL_SVM_DEVICES` outside of opencl)



Comment at: clang/test/SemaOpenCL/atomic-ops.cl:198
+
+void memory_fence_errors() {
+  __builtin_memory_fence(memory_order_seq_cst + 1, memory_scope_work_group); 
// expected-error {{memory order argument to fence operation is invalid}}

A happy case too please, e.g. to show that it accepts a couple of integers. 
Looks like ` __builtin_memory_fence(2, 2);` but without an expected-error 
comment


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75917/new/

https://reviews.llvm.org/D75917



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: clang/include/clang/Basic/Builtins.def:1516
 
+// Builtin to expose llvm fence instruction
+BUILTIN(__builtin_memory_fence, "vUiUi", "t")

`BUILTIN(__builtin_memory_fence, "vii", "n")`?

The other fence intrinsics (e.g. __c11_atomic_thread_fence) take signed 
integers and rely on the built in type checking, which seems reasonable here too


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75917/new/

https://reviews.llvm.org/D75917



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a subscriber: jdoerfert.
JonChesterfield added a comment.

@jdoerfert this is one of the two intrinsics needed to drop the .ll source from 
the amdgcn deviceRTL. The other is atomic_inc.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75917/new/

https://reviews.llvm.org/D75917



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

In D75917#1916972 , @sameerds wrote:

> Well, there is a problem: The LangRef says that scopes are target-defined. 
> This change says that scopes are defined by the high-level language and 
> further assumes that OpenCL scopes make sense in all languages. Besides 
> conflicting with the LangRef, this not seem to work with C++, which has no 
> scopes and nor with CUDA or HIP, whose scopes are not represented in any 
> AtomicScopeModel.


I don't follow. IR has a fence instruction. This builtin maps directly to it, 
passing whatever interfer arguments were given to the intrinsic along 
unchanged. It's exactly as valid, or invalid, as said fence instruction.

Are you objecting to passing enums in the test cases instead of raw integers?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75917/new/

https://reviews.llvm.org/D75917



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77774: [OpenMP] Allow to go first in C++-mode in target regions

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield accepted this revision.
JonChesterfield added a comment.
This revision is now accepted and ready to land.

The cmath/math.h story makes me sad, but this is a good workaround. Thanks


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D4/new/

https://reviews.llvm.org/D4



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

The tests look good, but I can't see the implementation in this diff. Maybe a 
file missing from the patch? Can be hard to tell with phabricator, the error 
may be at my end.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75917/new/

https://reviews.llvm.org/D75917



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13651
+  llvm::getConstantStringInfo(Scope, scp);
+  SSID = getLLVMContext().getOrInsertSyncScopeID(scp);
+

sameerds wrote:
> saiislam wrote:
> > sameerds wrote:
> > > This seems to be creating a new ID for any arbitrary string passed as 
> > > sync scope. This should be validated against 
> > > LLVMContext::getSyncScopeNames(). 
> > As the FE is not aware about specific target and implementation of sync 
> > scope for that target, getSyncScopeNames() here returns llvm'sdefault sync 
> > scopes, which only supports singlethreaded and system as valid scopes. 
> > Validity checking of memory scope string is being intentionally left for 
> > the later stages which deal with the generated IR.
> That's pretty strange. At this point, Clang should know what the target is, 
> and it should have a chance to update the list of sync scopes somewhere. 
> @arsenm, do you see a way around this?
There is already sufficient IR level checking for the string at the instruction 
level. Warning in clang as well could be a nicer user experience, but that 
seems low priority to me.



Comment at: clang/test/CodeGenHIP/builtin_memory_fence.cpp:9
+  // CHECK: fence syncscope("workgroup") seq_cst
+  __builtin_memory_fence(__ATOMIC_SEQ_CST,  "workgroup");
+  

sameerds wrote:
> JonChesterfield wrote:
> > saiislam wrote:
> > > sameerds wrote:
> > > > Orderings like `__ATOMIC_SEQ_CST` are defined for C/C++ memory models. 
> > > > They should not be used with the new builtin because this new builtin 
> > > > does not follow any specific language model. For user convenience, the 
> > > > right thing to do is to introduce new tokens in the Clang preprocessor, 
> > > > similar to the `__ATOMIC_*` tokens. The convenient shortcut is to just 
> > > > tell the user to supply numerical values by looking at the LLVM source 
> > > > code.
> > > > 
> > > > From llvm/Support/AtomicOrdering.h, note how the numerical value for 
> > > > `__ATOMIC_SEQ_CST` is 5, but the numerical value for the LLVM 
> > > > SequentiallyConsistent ordering is 7. The numerical value 5 refers to 
> > > > the LLVM ordering "release". So, if the implementation were correct, 
> > > > this line should result in the following unexpected LLVM IR:
> > > >   fence syncscope("workgroup") release
> > > As you pointed out, the range of acquire to sequentiallly consistent 
> > > memory orders for llvm::AtomicOrdering is [4, 7], while for 
> > > llvm::AtomicOrderingCABI is [2, 5]. Enums of C ABI was taken to ensure 
> > > easy of use for the users who are familiar with C/C++ standard memory 
> > > model. It allows them to use macros like __ATOMIC_ACQUIRE etc.
> > > Clang CodeGen of the builtin internally maps C ABI ordering to llvm 
> > > atomic ordering.
> > What language, implemented in clang, do you have in mind that reusing the 
> > existing __ATOMIC_* macros would be incorrect for?
> I think we agreed that this builtin exposes the LLVM fence exactly. That 
> would mean it takes arguments defined by LLVM. If you are implementing 
> something different from that, then it first needs to be specified properly. 
> Perhaps you could say that this is a C ABI compatible builtin, that happens 
> to take target specific scopes? That should cover OpenCL whose scope enum is 
> designed to be compatible with C.
> 
> Whatever it is that you are trying to implement here, it definitely does not 
> expose a raw LLVM fence.
The llvm fence, in text form, uses a symbol for the memory scope. Not an enum.

This symbol is set using these macros for the existing atomic builtins. Using 
an implementation detail of clang instead is strictly worse, by layering and by 
precedent.

ABI is not involved here. Nor is OpenCl.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75917/new/

https://reviews.llvm.org/D75917



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: clang/test/CodeGenHIP/builtin_memory_fence.cpp:9
+  // CHECK: fence syncscope("workgroup") seq_cst
+  __builtin_memory_fence(__ATOMIC_SEQ_CST,  "workgroup");
+  

JonChesterfield wrote:
> sameerds wrote:
> > JonChesterfield wrote:
> > > sameerds wrote:
> > > > JonChesterfield wrote:
> > > > > saiislam wrote:
> > > > > > sameerds wrote:
> > > > > > > Orderings like `__ATOMIC_SEQ_CST` are defined for C/C++ memory 
> > > > > > > models. They should not be used with the new builtin because this 
> > > > > > > new builtin does not follow any specific language model. For user 
> > > > > > > convenience, the right thing to do is to introduce new tokens in 
> > > > > > > the Clang preprocessor, similar to the `__ATOMIC_*` tokens. The 
> > > > > > > convenient shortcut is to just tell the user to supply numerical 
> > > > > > > values by looking at the LLVM source code.
> > > > > > > 
> > > > > > > From llvm/Support/AtomicOrdering.h, note how the numerical value 
> > > > > > > for `__ATOMIC_SEQ_CST` is 5, but the numerical value for the LLVM 
> > > > > > > SequentiallyConsistent ordering is 7. The numerical value 5 
> > > > > > > refers to the LLVM ordering "release". So, if the implementation 
> > > > > > > were correct, this line should result in the following unexpected 
> > > > > > > LLVM IR:
> > > > > > >   fence syncscope("workgroup") release
> > > > > > As you pointed out, the range of acquire to sequentiallly 
> > > > > > consistent memory orders for llvm::AtomicOrdering is [4, 7], while 
> > > > > > for llvm::AtomicOrderingCABI is [2, 5]. Enums of C ABI was taken to 
> > > > > > ensure easy of use for the users who are familiar with C/C++ 
> > > > > > standard memory model. It allows them to use macros like 
> > > > > > __ATOMIC_ACQUIRE etc.
> > > > > > Clang CodeGen of the builtin internally maps C ABI ordering to llvm 
> > > > > > atomic ordering.
> > > > > What language, implemented in clang, do you have in mind that reusing 
> > > > > the existing __ATOMIC_* macros would be incorrect for?
> > > > I think we agreed that this builtin exposes the LLVM fence exactly. 
> > > > That would mean it takes arguments defined by LLVM. If you are 
> > > > implementing something different from that, then it first needs to be 
> > > > specified properly. Perhaps you could say that this is a C ABI 
> > > > compatible builtin, that happens to take target specific scopes? That 
> > > > should cover OpenCL whose scope enum is designed to be compatible with 
> > > > C.
> > > > 
> > > > Whatever it is that you are trying to implement here, it definitely 
> > > > does not expose a raw LLVM fence.
> > > The llvm fence, in text form, uses a symbol for the memory scope. Not an 
> > > enum.
> > > 
> > > This symbol is set using these macros for the existing atomic builtins. 
> > > Using an implementation detail of clang instead is strictly worse, by 
> > > layering and by precedent.
> > > 
> > > ABI is not involved here. Nor is OpenCl.
> > The `__ATOMIC_*` symbols in Clang quite literally represent the C/C++ ABI. 
> > See the details in AtomicOrdering.h and InitPreprocessor.cpp. I am not 
> > opposed to specifying that the builtin expects these symbols, but then it 
> > is incorrect to say that the builtin exposes the raw LLVM builtin. It is a 
> > C-ABI-compatible builtin that happens to take target-specific scope as a 
> > string argument. And that would also make it an overload of the already 
> > existing builting __atomic_fence().
> I don't know what you mean by "raw",  but am guessing you're asking for 
> documentation for the intrinsic. Said documentation should indeed be added 
> for this builtin - it'll probably be in a tablegen file.
I will try to stop using builtin and intrinsic as synonyms.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75917/new/

https://reviews.llvm.org/D75917



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: clang/test/CodeGenHIP/builtin_memory_fence.cpp:1
 // REQUIRES: amdgpu-registered-target
 // RUN: %clang_cc1 %s -x hip -emit-llvm -O0 -o - \

Codegen test should be under CodeGen and/or CodeGenCXX


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75917/new/

https://reviews.llvm.org/D75917



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: clang/test/CodeGenHIP/builtin_memory_fence.cpp:9
+  // CHECK: fence syncscope("workgroup") seq_cst
+  __builtin_memory_fence(__ATOMIC_SEQ_CST,  "workgroup");
+  

sameerds wrote:
> JonChesterfield wrote:
> > sameerds wrote:
> > > JonChesterfield wrote:
> > > > saiislam wrote:
> > > > > sameerds wrote:
> > > > > > Orderings like `__ATOMIC_SEQ_CST` are defined for C/C++ memory 
> > > > > > models. They should not be used with the new builtin because this 
> > > > > > new builtin does not follow any specific language model. For user 
> > > > > > convenience, the right thing to do is to introduce new tokens in 
> > > > > > the Clang preprocessor, similar to the `__ATOMIC_*` tokens. The 
> > > > > > convenient shortcut is to just tell the user to supply numerical 
> > > > > > values by looking at the LLVM source code.
> > > > > > 
> > > > > > From llvm/Support/AtomicOrdering.h, note how the numerical value 
> > > > > > for `__ATOMIC_SEQ_CST` is 5, but the numerical value for the LLVM 
> > > > > > SequentiallyConsistent ordering is 7. The numerical value 5 refers 
> > > > > > to the LLVM ordering "release". So, if the implementation were 
> > > > > > correct, this line should result in the following unexpected LLVM 
> > > > > > IR:
> > > > > >   fence syncscope("workgroup") release
> > > > > As you pointed out, the range of acquire to sequentiallly consistent 
> > > > > memory orders for llvm::AtomicOrdering is [4, 7], while for 
> > > > > llvm::AtomicOrderingCABI is [2, 5]. Enums of C ABI was taken to 
> > > > > ensure easy of use for the users who are familiar with C/C++ standard 
> > > > > memory model. It allows them to use macros like __ATOMIC_ACQUIRE etc.
> > > > > Clang CodeGen of the builtin internally maps C ABI ordering to llvm 
> > > > > atomic ordering.
> > > > What language, implemented in clang, do you have in mind that reusing 
> > > > the existing __ATOMIC_* macros would be incorrect for?
> > > I think we agreed that this builtin exposes the LLVM fence exactly. That 
> > > would mean it takes arguments defined by LLVM. If you are implementing 
> > > something different from that, then it first needs to be specified 
> > > properly. Perhaps you could say that this is a C ABI compatible builtin, 
> > > that happens to take target specific scopes? That should cover OpenCL 
> > > whose scope enum is designed to be compatible with C.
> > > 
> > > Whatever it is that you are trying to implement here, it definitely does 
> > > not expose a raw LLVM fence.
> > The llvm fence, in text form, uses a symbol for the memory scope. Not an 
> > enum.
> > 
> > This symbol is set using these macros for the existing atomic builtins. 
> > Using an implementation detail of clang instead is strictly worse, by 
> > layering and by precedent.
> > 
> > ABI is not involved here. Nor is OpenCl.
> The `__ATOMIC_*` symbols in Clang quite literally represent the C/C++ ABI. 
> See the details in AtomicOrdering.h and InitPreprocessor.cpp. I am not 
> opposed to specifying that the builtin expects these symbols, but then it is 
> incorrect to say that the builtin exposes the raw LLVM builtin. It is a 
> C-ABI-compatible builtin that happens to take target-specific scope as a 
> string argument. And that would also make it an overload of the already 
> existing builting __atomic_fence().
I don't know what you mean by "raw",  but am guessing you're asking for 
documentation for the intrinsic. Said documentation should indeed be added for 
this builtin - it'll probably be in a tablegen file.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75917/new/

https://reviews.llvm.org/D75917



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77918: [OpenMP] Avoid crash in preparation for diagnose of unsupported type

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield accepted this revision.
JonChesterfield added a comment.
This revision is now accepted and ready to land.

Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77918/new/

https://reviews.llvm.org/D77918



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77390: Fix __builtin_amdgcn_workgroup_size_x/y/z return type

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: clang/test/CodeGenOpenCL/builtins-amdgcn.cl:541
switch (d) {
-   case 0: *out = __builtin_amdgcn_workgroup_size_x(); break;
+   case 0: *out = __builtin_amdgcn_workgroup_size_x() + 1; break;
case 1: *out = __builtin_amdgcn_workgroup_size_y(); break;

This looks unrelated to the return type change


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77390/new/

https://reviews.llvm.org/D77390



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78495: [nfc] Accept addrspacecast allocas in InitTempAlloca

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield abandoned this revision.
JonChesterfield added a comment.

No problem. This isn't on the live path - the function is mostly called from 
openmp codegen and clang doesn't target openmp/amdgcn just yet. I'll roll this 
change into the codegen patch to enable that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78495/new/

https://reviews.llvm.org/D78495



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78495: [nfc] Accept addrspacecast allocas in InitTempAlloca

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

In D78495#1992404 , @arsenm wrote:

> Needs test?


I'm not sure how to write said test. How do we normally hit asserts from the 
clang test suite?

This fires a lot in the openmp on amdgcn downstream branch, but I'm happy 
carrying this as a local patch until the rest of the clang change can be put up 
for review if preferred.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78495/new/

https://reviews.llvm.org/D78495



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78495: [nfc] Accept addrspacecast allocas in InitTempAlloca

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield created this revision.
JonChesterfield added reviewers: rjmccall, aaron.ballman, ABataev, jdoerfert, 
arsenm.
Herald added subscribers: cfe-commits, wdng.
Herald added a project: clang.

[nfc] Accept addrspacecast allocas in InitTempAlloca
Changes the precondition to be slightly more permissive. Useful for amdgcn where
allocas are created with a cast to an address space.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78495

Files:
  clang/lib/CodeGen/CGExpr.cpp


Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -125,8 +125,12 @@
 }
 
 void CodeGenFunction::InitTempAlloca(Address Var, llvm::Value *Init) {
-  assert(isa(Var.getPointer()));
-  auto *Store = new llvm::StoreInst(Init, Var.getPointer());
+  auto *Alloca = Var.getPointer();
+  assert(isa(Alloca) ||
+ (isa(Alloca) &&
+  isa(
+  cast(Alloca)->getPointerOperand(;
+  auto *Store = new llvm::StoreInst(Init, Alloca);
   Store->setAlignment(Var.getAlignment().getAsAlign());
   llvm::BasicBlock *Block = AllocaInsertPt->getParent();
   Block->getInstList().insertAfter(AllocaInsertPt->getIterator(), Store);


Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -125,8 +125,12 @@
 }
 
 void CodeGenFunction::InitTempAlloca(Address Var, llvm::Value *Init) {
-  assert(isa(Var.getPointer()));
-  auto *Store = new llvm::StoreInst(Init, Var.getPointer());
+  auto *Alloca = Var.getPointer();
+  assert(isa(Alloca) ||
+ (isa(Alloca) &&
+  isa(
+  cast(Alloca)->getPointerOperand(;
+  auto *Store = new llvm::StoreInst(Init, Alloca);
   Store->setAlignment(Var.getAlignment().getAsAlign());
   llvm::BasicBlock *Block = AllocaInsertPt->getParent();
   Block->getInstList().insertAfter(AllocaInsertPt->getIterator(), Store);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74361: [Clang] Undef attribute for global variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield updated this revision to Diff 250263.
JonChesterfield added a comment.

- Amend diagnostic as suggested, clang-format new lines in SemaKinds.td


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74361/new/

https://reviews.llvm.org/D74361

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/DeclBase.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-loader-uninitialized.c
  clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-loader-uninitialized.c
  clang/test/Sema/attr-loader-uninitialized.cpp

Index: clang/test/Sema/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.cpp
@@ -0,0 +1,60 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only
+
+int good __attribute__((loader_uninitialized));
+static int local_ok __attribute__((loader_uninitialized));
+int hidden_ok __attribute__((visibility("hidden"))) __attribute__((loader_uninitialized));
+
+const int still_cant_be_const __attribute__((loader_uninitialized));
+// expected-error@-1 {{default initialization of an object of const type}}
+extern int external_rejected __attribute__((loader_uninitialized));
+// expected-error@-1 {{variable 'external_rejected' cannot be declared both 'extern' and with the 'loader_uninitialized' attribute}}
+
+int noargs __attribute__((loader_uninitialized(0)));
+// expected-error@-1 {{'loader_uninitialized' attribute takes no arguments}} 
+
+int init_rejected __attribute__((loader_uninitialized)) = 42;
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute cannot have an initializer}}
+
+void func() __attribute__((loader_uninitialized))
+// expected-warning@-1 {{'loader_uninitialized' attribute only applies to global variables}}
+{
+  int local __attribute__((loader_uninitialized));
+  // expected-warning@-1 {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static int sl __attribute__((loader_uninitialized));
+}
+
+struct s {
+  __attribute__((loader_uninitialized)) int field;
+  // expected-warning@-1 {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static __attribute__((loader_uninitialized)) int sfield;
+
+} __attribute__((loader_uninitialized));
+// expected-warning@-1 {{'loader_uninitialized' attribute only applies to global variables}}
+
+int redef_attr_first __attribute__((loader_uninitialized));
+int redef_attr_first;
+// expected-error@-1 {{redefinition of 'redef_attr_first'}}
+// expected-note@-3 {{previous definition is here}}
+
+int redef_attr_second; 
+int redef_attr_second __attribute__((loader_uninitialized)); 
+// expected-warning@-1 {{attribute declaration must precede definition}}
+// expected-note@-3 {{previous definition is here}}
+// expected-error@-3 {{redefinition of 'redef_attr_second'}}
+// expected-note@-5 {{previous definition is here}}
+
+struct trivial {};
+
+trivial default_ok __attribute__((loader_uninitialized));
+trivial value_rejected  __attribute__((loader_uninitialized)) {};
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute cannot have an initializer}}
+
+struct nontrivial
+{
+  nontrivial() {}
+};
+
+nontrivial needs_trivial_ctor __attribute__((loader_uninitialized));
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute must have a trivial default constructor}}
Index: clang/test/Sema/attr-loader-uninitialized.c
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.c
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only
+// See also attr-loader-uninitialized.cpp
+
+int good __attribute__((loader_uninitialized));
+static int local_ok __attribute__((loader_uninitialized));
+int hidden_ok __attribute__((visibility("hidden"))) __attribute__((loader_uninitialized));
+
+const int can_still_be_const __attribute__((loader_uninitialized));
+
+extern int external_rejected __attribute__((loader_uninitialized));
+// expected-error@-1 {{variable 'external_rejected' cannot be declared both 'extern' and with the 'loader_uninitialized' attribute}}
+
+int noargs __attribute__((loader_uninitialized(0)));
+// expected-error@-1 {{'loader_uninitialized' attribute takes no arguments}}
+
+int init_rejected __attribute__((loader_uninitialized)) = 42;
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute cannot have an initializer}}
+
+int declaration_then_uninit_ok;
+int declaration_then_uninit_ok __attribute__((loader_uninitialized));
+
+int definition_then_uninit_rejected = 0;
+int definition_then_uninit_rejected __attribute__((loader_uninitialized));
+// 

[PATCH] D74361: [Clang] Undef attribute for global variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

In D74361#1920329 , @aaron.ballman 
wrote:

> Aside from the diagnostic wording, I think this LG to me. However, I'd 
> appreciate if @rjmccall would also sign off.


Thanks! @rjmccall?




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5344
+def err_loader_uninitialized_extern_decl : Error<
+  "external declaration of variable cannot have the 'loader_uninitialized' 
attribute">;
 def err_block_extern_cant_init : Error<

aaron.ballman wrote:
> How would you feel about: `"variable %0 cannot be declared both 'extern' and 
> with the 'loader_uninitialized' attribute"` (or something along those lines) 
> to clarify "external declaration"?
Better, thanks. Also discovered clang-format has learned to do sensible things 
with tablegen so applied it to the new defs.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74361/new/

https://reviews.llvm.org/D74361



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74361: [Clang] Undef attribute for global variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield updated this revision to Diff 250264.
JonChesterfield marked an inline comment as done.
JonChesterfield added a comment.

- undo reformat of existing def


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74361/new/

https://reviews.llvm.org/D74361

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/DeclBase.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-loader-uninitialized.c
  clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-loader-uninitialized.c
  clang/test/Sema/attr-loader-uninitialized.cpp

Index: clang/test/Sema/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.cpp
@@ -0,0 +1,60 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only
+
+int good __attribute__((loader_uninitialized));
+static int local_ok __attribute__((loader_uninitialized));
+int hidden_ok __attribute__((visibility("hidden"))) __attribute__((loader_uninitialized));
+
+const int still_cant_be_const __attribute__((loader_uninitialized));
+// expected-error@-1 {{default initialization of an object of const type}}
+extern int external_rejected __attribute__((loader_uninitialized));
+// expected-error@-1 {{variable 'external_rejected' cannot be declared both 'extern' and with the 'loader_uninitialized' attribute}}
+
+int noargs __attribute__((loader_uninitialized(0)));
+// expected-error@-1 {{'loader_uninitialized' attribute takes no arguments}} 
+
+int init_rejected __attribute__((loader_uninitialized)) = 42;
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute cannot have an initializer}}
+
+void func() __attribute__((loader_uninitialized))
+// expected-warning@-1 {{'loader_uninitialized' attribute only applies to global variables}}
+{
+  int local __attribute__((loader_uninitialized));
+  // expected-warning@-1 {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static int sl __attribute__((loader_uninitialized));
+}
+
+struct s {
+  __attribute__((loader_uninitialized)) int field;
+  // expected-warning@-1 {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static __attribute__((loader_uninitialized)) int sfield;
+
+} __attribute__((loader_uninitialized));
+// expected-warning@-1 {{'loader_uninitialized' attribute only applies to global variables}}
+
+int redef_attr_first __attribute__((loader_uninitialized));
+int redef_attr_first;
+// expected-error@-1 {{redefinition of 'redef_attr_first'}}
+// expected-note@-3 {{previous definition is here}}
+
+int redef_attr_second; 
+int redef_attr_second __attribute__((loader_uninitialized)); 
+// expected-warning@-1 {{attribute declaration must precede definition}}
+// expected-note@-3 {{previous definition is here}}
+// expected-error@-3 {{redefinition of 'redef_attr_second'}}
+// expected-note@-5 {{previous definition is here}}
+
+struct trivial {};
+
+trivial default_ok __attribute__((loader_uninitialized));
+trivial value_rejected  __attribute__((loader_uninitialized)) {};
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute cannot have an initializer}}
+
+struct nontrivial
+{
+  nontrivial() {}
+};
+
+nontrivial needs_trivial_ctor __attribute__((loader_uninitialized));
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute must have a trivial default constructor}}
Index: clang/test/Sema/attr-loader-uninitialized.c
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.c
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only
+// See also attr-loader-uninitialized.cpp
+
+int good __attribute__((loader_uninitialized));
+static int local_ok __attribute__((loader_uninitialized));
+int hidden_ok __attribute__((visibility("hidden"))) __attribute__((loader_uninitialized));
+
+const int can_still_be_const __attribute__((loader_uninitialized));
+
+extern int external_rejected __attribute__((loader_uninitialized));
+// expected-error@-1 {{variable 'external_rejected' cannot be declared both 'extern' and with the 'loader_uninitialized' attribute}}
+
+int noargs __attribute__((loader_uninitialized(0)));
+// expected-error@-1 {{'loader_uninitialized' attribute takes no arguments}}
+
+int init_rejected __attribute__((loader_uninitialized)) = 42;
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute cannot have an initializer}}
+
+int declaration_then_uninit_ok;
+int declaration_then_uninit_ok __attribute__((loader_uninitialized));
+
+int definition_then_uninit_rejected = 0;
+int definition_then_uninit_rejected __attribute__((loader_uninitialized));

[PATCH] D76342: [OpenMP] Implement '#pragma omp tile'

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

edit: actually you've already done the clang-format on trunk as I hoped, 
phabricator mislead me. Apologies for the noise


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76342/new/

https://reviews.llvm.org/D76342



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

Can this be revived? Changing the enum to a string still sounds good to me


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75917/new/

https://reviews.llvm.org/D75917



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77113: [OpenMP][NFC] Move and simplify directive -> allowed clause mapping

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield accepted this revision.
JonChesterfield added a comment.
This revision is now accepted and ready to land.

LGTM too. Non functional change, clearer code. Thanks


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77113/new/

https://reviews.llvm.org/D77113



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:1888
+// Check valididty of memory ordering as per C11 / C++11's memody model.
+if (ord < static_cast(llvm::AtomicOrderingCABI::acquire) ||
+  ord > static_cast(llvm::AtomicOrderingCABI::seq_cst)) {

I think I'd write this as a switch over the enum instead of a ranged compare.

It'll codegen to the same thing, but we'll get warnings if more values are 
introduced to the enum and things will keep working (here, anyway) if the 
values are reordered.



Comment at: clang/test/CodeGenHIP/builtin_memory_fence.cpp:9
+  // CHECK: fence syncscope("workgroup") seq_cst
+  __builtin_memory_fence(__ATOMIC_SEQ_CST,  "workgroup");
+  

saiislam wrote:
> sameerds wrote:
> > Orderings like `__ATOMIC_SEQ_CST` are defined for C/C++ memory models. They 
> > should not be used with the new builtin because this new builtin does not 
> > follow any specific language model. For user convenience, the right thing 
> > to do is to introduce new tokens in the Clang preprocessor, similar to the 
> > `__ATOMIC_*` tokens. The convenient shortcut is to just tell the user to 
> > supply numerical values by looking at the LLVM source code.
> > 
> > From llvm/Support/AtomicOrdering.h, note how the numerical value for 
> > `__ATOMIC_SEQ_CST` is 5, but the numerical value for the LLVM 
> > SequentiallyConsistent ordering is 7. The numerical value 5 refers to the 
> > LLVM ordering "release". So, if the implementation were correct, this line 
> > should result in the following unexpected LLVM IR:
> >   fence syncscope("workgroup") release
> As you pointed out, the range of acquire to sequentiallly consistent memory 
> orders for llvm::AtomicOrdering is [4, 7], while for llvm::AtomicOrderingCABI 
> is [2, 5]. Enums of C ABI was taken to ensure easy of use for the users who 
> are familiar with C/C++ standard memory model. It allows them to use macros 
> like __ATOMIC_ACQUIRE etc.
> Clang CodeGen of the builtin internally maps C ABI ordering to llvm atomic 
> ordering.
What language, implemented in clang, do you have in mind that reusing the 
existing __ATOMIC_* macros would be incorrect for?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75917/new/

https://reviews.llvm.org/D75917



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

Amdgcn specific is fine by me. Hopefully that unblocks this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75917/new/

https://reviews.llvm.org/D75917



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: clang/test/CodeGenCXX/builtin-amdgcn-fence-failure.cpp:5
+
+void test_amdgcn_fence_failure() {
+

arsenm wrote:
> Does this really depend on C++? Can it use OpenCL like the other builtin 
> tests?This also belongs in a Sema* test directory since it's checking an error
Making it opencl-only would force some of the openmp runtime to be written in 
opencl, which is not presently the case. Currently that library is written in a 
dialect of hip, but there's a plan to implement it in openmp instead.

I'd much rather this builtin work from any language, instead of tying it to 
opencl, as that means one can use it from openmp target regions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75917/new/

https://reviews.llvm.org/D75917



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits