[PATCH] D50845: [CUDA/OpenMP] Define only some host macros during device compilation

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

> Another option would be to implement some sort of attribute-based 
> overloading. Then OpenMP can provide its own version of the device-side 
> library function without clashing with system headers.

I'm thinking about what the desired behavior is here. So, if we have a 
situation where the target is the host, then we really only have one set of 
headers and we want everything to work as it does today. math.h should be 
math.h, with the same relevant preprocessor context. When the host and target 
differ, then we have a restricted set of external/system device functions 
available on the device (which may or may not have anything to do with the set 
of system functions provided by the host's system headers). As a result, we 
should really have a separate header that has those actually-available 
functions. When targeting NVPTX, why don't we have the included math.h be 
CUDA's math.h? In the end, those are the functions we need to call when we 
generate code. Right?


Repository:
  rC Clang

https://reviews.llvm.org/D50845



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

[PATCH] D50845: [CUDA/OpenMP] Define only some host macros during device compilation

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D50845#1202965, @Hahnfeld wrote:

> In https://reviews.llvm.org/D50845#1202963, @hfinkel wrote:
>
> > As a result, we should really have a separate header that has those 
> > actually-available functions. When targeting NVPTX, why don't we have the 
> > included math.h be CUDA's math.h? In the end, those are the functions we 
> > need to call when we generate code. Right?
>
>
> That's what https://reviews.llvm.org/D47849 deals with.


Yes, but it doesn't get CUDA's math.h. Maybe I misunderstand how this works 
(and I very well might, because it's not clear that CUDA has a math.h by that 
name), but that patch tries to avoid problems with the host's math.h and then 
also injects __clang_cuda_device_functions.h into the device compilation. How 
does this compare to when you include math.h in Clang's CUDA mode? It seems to 
be that we want to somehow map standard includes, where applicable, to include 
files in CUDA's include/crt directory (e.g., crt/math_functions.h and 
crt/common_functions.h for stdio.h for printf), and nothing else ends up being 
available (because it is, in fact, not available).


Repository:
  rC Clang

https://reviews.llvm.org/D50845



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

[PATCH] D39053: [Bitfield] Add more cases to making the bitfield a separate location

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D39053#1093977, @asb wrote:

> Just wanted to explicitly say that I'm happy the updated patch reflects the 
> changes to docs and comments I requested. @hfinkel - are you happy for this 
> to land now?


Yes, go ahead.

Some of these benefits might still be capturable using the regular method and 
backend improvements, but with this under the option it should be easier to 
identify where those opportunities are.


https://reviews.llvm.org/D39053



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

[PATCH] D47267: [UnrollAndJam] Add unroll_and_jam pragma handling

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

> I have some doubts whether this will ever be used sensibly in the real world, 
> but can
>  be useful for testing and was not difficult to put together.

I definitely support having pragmas for these kinds of loop transformations. In 
my experience, they are used.


https://reviews.llvm.org/D47267



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

[PATCH] D45835: Add new driver mode for dumping compiler options

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added inline comments.



Comment at: test/Frontend/compiler-options-dump.cpp:3
+// RUN: %clang_cc1 -compiler-options-dump -std=c++17 %s -o - | FileCheck %s 
--check-prefix=CXX17
+// RUN: %clang_cc1 -compiler-options-dump -std=c99 -ffast-math -x c %s -o - | 
FileCheck %s --check-prefix=C99
+

You don't need -ffast-math here I presume.



Comment at: test/Frontend/compiler-options-dump.cpp:15
+// CXX17: "extensions"
+// CXX17: "cxx_range_for" : true
+

cxx_range_for is both a feature and an extension?


https://reviews.llvm.org/D45835



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

[PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

You seem to be only changing the behavior for the "separatable" fields, but I 
suspect you want to change the behavior for the others too. The bitfield would 
be decomposed into shards, separated by the naturally-sized-and-aligned fields. 
Each access only loads its shard. For example, in your test case you have:

  struct S3 {
unsigned long f1:14;
unsigned long f2:18;
unsigned long f3:32;
  };

and you test that, with this option, loading/storing to a3.f3 only access the 
specific 4 bytes composing f3. But if you load f1 or f2, we're still loading 
all 8 bytes, right? I think we should only load/store the lower 4 bytes when we 
access a3.f1 and/or a3.f2.

Otherwise, you can again end up with the narrow-store/wide-load problem for 
nearby fields under a different set of circumstances.




Comment at: include/clang/Driver/Options.td:1032
 
+def fsplit_bitfields : Flag<["-"], "fsplit-bitfields">,
+  Group, Flags<[CC1Option]>,

I'm not opposed to -fsplit-bitfields, but I'd prefer if we find something more 
self-explanatory. It's not really clear what "splitting a bitfield" means. 
Maybe?

  -fsplit-bitfield-accesses
  -fdecomposed-bitfield-accesses
  -fsharded-bitfield-accesses
  -ffine-grained-bitfield-accesses

(I think that I prefer -ffine-grained-bitfield-accesses, although it's the 
longest)



Comment at: include/clang/Driver/Options.td:1034
+  Group, Flags<[CC1Option]>,
+  HelpText<"Enable splitting bitfield with legal width and alignment in 
LLVM.">;
+def fno_split_bitfields : Flag<["-"], "fno-split-bitfields">,

How about?

  Use separate access for bitfields with legal widths and alignments.

I don't think that "in LLVM" is needed here (or we could put "in LLVM" on an 
awful lot of these options).



Comment at: lib/CodeGen/CGExpr.cpp:1679
+/// field has legal integer width, and its bitfield offset is naturally
+/// aligned, it is better to access the bitfield like a separate integer var.
+static bool IsSeparatableBitfield(const CGBitFieldInfo &Info,

var -> variable


Repository:
  rL LLVM

https://reviews.llvm.org/D36562



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

[PATCH] D38113: OpenCL: Assume functions are convergent

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D38113#882187, @Anastasia wrote:

> In https://reviews.llvm.org/D38113#878840, @jlebar wrote:
>
> >
>


...

> Yes, I see this sounds more reasonable indeed. Btw, currently LLVM can remove 
> `convergent` in some cases to recover the performance loss?

Yes. See removeConvergentAttrs in lib/Transforms/IPO/FunctionAttrs.cpp.

...


https://reviews.llvm.org/D38113



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

[PATCH] D38186: Add the new -Wnull-pointer-arithmetic warnings to the release notes

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added inline comments.



Comment at: ReleaseNotes.rst:82
+- ``-Wnull-pointer-arithmetic`` now warns about performing pointer arithmetic
+  on a null pointer. It has an undefined behavior if the offset is nonzero.
+

We can probably just make this one entry:

  ``-Wnull-pointer-arithmetic`` now warns about performing pointer arithmetic 
on a null pointer. Such pointer arithmetic has an undefined behavior if the 
offset is nonzero. It also now warns about arithmetic on a null pointer treated 
as a cast from integer to pointer (GNU extension).



https://reviews.llvm.org/D38186



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

[PATCH] D37624: add support for -fno-instrument-functions and -finstrument-functions-exclude-{file, function}-list= to match gcc options.

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

Please also add a C++ test to check the mangling-related features.




Comment at: lib/CodeGen/CodeGenFunction.cpp:419
+// Assume that __cxa_demangle is provided by libcxxabi (except for Windows).
+extern "C" char *__cxa_demangle(const char *mangled_name, char *output_buffer,
+size_t *length, int *status);

Using the system demangler here seems like the wrong solution. You have the 
current function declaration, `CurFuncDecl`, and from it you should be able to 
extract the full name.

I think that this will do it:

  std::string FullFuncName = PredefinedExpr::ComputeName(
  PredefinedExpr::PrettyFunctionNoVirtual, CurFuncDecl);




Comment at: lib/CodeGen/CodeGenModule.cpp:4601
+CodeGenModule::GetSourceLocToFileNameMap() {
+  return SourceLocToFileNameMap;
+}

Make this function inline in the class definition.


https://reviews.llvm.org/D37624



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

[PATCH] D37624: add support for -fno-instrument-functions and -finstrument-functions-exclude-{file, function}-list= to match gcc options.

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added inline comments.



Comment at: test/CodeGenCXX/instrument-functions.cpp:8
+// RUN: %clang -S -emit-llvm -o - %s -finstrument-functions 
-finstrument-functions-exclude-function-list=test3 | FileCheck %s 
--check-prefix=NOFUNC
+// RUN: %clang -S -emit-llvm -o - %s -finstrument-functions 
-finstrument-functions-exclude-function-list=Z5test3 | FileCheck %s 
--check-prefix=NOFUNC2
+

I'm a bit confused about this test. Are you trying to match against the mangled 
names, or the demangled names, or both?


https://reviews.llvm.org/D37624



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

[PATCH] D37624: add support for -fno-instrument-functions and -finstrument-functions-exclude-{file, function}-list= to match gcc options.

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D37624#885288, @choikwa wrote:

> - add comment to CPP test to explain usage


Thanks. Please also add some tests showing matching overloaded functions, 
functions with template parameters, etc.

Do we need to strip whitespace before trying to match the demangled names?


https://reviews.llvm.org/D37624



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

[PATCH] D37624: add support for -fno-instrument-functions and -finstrument-functions-exclude-{file, function}-list= to match gcc options.

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D37624#885295, @choikwa wrote:

> In https://reviews.llvm.org/D37624#885290, @hfinkel wrote:
>
> > In https://reviews.llvm.org/D37624#885288, @choikwa wrote:
> >
> > > - add comment to CPP test to explain usage
> >
> >
> > Thanks. Please also add some tests showing matching overloaded functions, 
> > functions with template parameters, etc.
> >
> > Do we need to strip whitespace before trying to match the demangled names?
>
>
> Some cursory testing with g++ shows that only the 'test5' of 'test5(float, 
> int, int*)' is matched. 'test5(' or 'test5 (' is not matched. It seems weird 
> that arguments are not matched.
>
> g++ man page shows
>
>   "The function name to be matched is its user-visible name, such as 
> "vector blah(const vector &)", not the internal mangled name"
>
> but it doesn't seem to be including parameters.


Interesting. Can you tell what GCC is doing w.r.t. namespace names, class 
names, etc. and template parameters?

> Also uncovered a bug where sub argument list containing comma needs to be 
> surrounded by single quote, but clang seems to ignores single quote.
>  I'll try to dig around ArgList implementation to see if it can return 
> argument surrounded by single-quote as a whole.




https://reviews.llvm.org/D37624



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

[PATCH] D37624: add support for -fno-instrument-functions and -finstrument-functions-exclude-{file, function}-list= to match gcc options.

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D37624#885311, @choikwa wrote:

> In https://reviews.llvm.org/D37624#885308, @hfinkel wrote:
>
> > In https://reviews.llvm.org/D37624#885295, @choikwa wrote:
> >
> > > In https://reviews.llvm.org/D37624#885290, @hfinkel wrote:
> > >
> > > > In https://reviews.llvm.org/D37624#885288, @choikwa wrote:
> > > >
> > > > > - add comment to CPP test to explain usage
> > > >
> > > >
> > > > Thanks. Please also add some tests showing matching overloaded 
> > > > functions, functions with template parameters, etc.
> > > >
> > > > Do we need to strip whitespace before trying to match the demangled 
> > > > names?
> > >
> > >
> > > Some cursory testing with g++ shows that only the 'test5' of 
> > > 'test5(float, int, int*)' is matched. 'test5(' or 'test5 (' is not 
> > > matched. It seems weird that arguments are not matched.
> > >
> > > g++ man page shows
> > >
> > >   "The function name to be matched is its user-visible name, such as 
> > > "vector blah(const vector &)", not the internal mangled name"
> > >
> > > but it doesn't seem to be including parameters.
> >
> >
> > Interesting. Can you tell what GCC is doing w.r.t. namespace names, class 
> > names, etc. and template parameters?
> >
> > > Also uncovered a bug where sub argument list containing comma needs to be 
> > > surrounded by single quote, but clang seems to ignores single quote.
> > >  I'll try to dig around ArgList implementation to see if it can return 
> > > argument surrounded by single-quote as a whole.
>
>
> Given A::B::C(T a), only 'C' is meaningful in g++'s matcher. Adding 
> anything else escapes the match [  'B::C', 'C('  ].
>  It seems like g++ will also not match single quotes as a whole, ie. int 
> fooboo() is matched by 'foo,boo'.


Can you get more information on what GCC actually implemented and why? It's not 
clear to me that ignoring the namespaces is the most-useful way to do this. I 
don't want to emulate GCC bugs, but maybe there's a good reason why their 
implementation works this way.


https://reviews.llvm.org/D37624



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

[PATCH] D31417: [OpenMP] Add support for omp simd pragmas without runtime

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

Is this still being worked on?


https://reviews.llvm.org/D31417



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

[PATCH] D37624: add support for -fno-instrument-functions and -finstrument-functions-exclude-{file, function}-list= to match gcc options.

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D37624#885438, @choikwa wrote:

> > Can you get more information on what GCC actually implemented and why? It's 
> > not clear to me that ignoring the namespaces is the most-useful way to do 
> > this. I don't want to emulate GCC bugs, but maybe there's a good reason why 
> > their implementation works this way.
>
> This is what I found, https://gcc.gnu.org/ml/gcc-patches/2015-12/msg00473.html


To state the obvious, that patch has zero C++ test cases. Can try emailing the 
author of the feature in GCC and try to get some clarity on what the intended 
behavior is on their end? I'm trying to figure out what's useful here. I think 
we do want to match namespaces.

> Diff shows it doesn't seem to specially treat single quotes.
> 
> +case OPT_finstrument_functions_include_function_list_:
>  +  add_comma_separated_to_vector
>  +(&opts->x_flag_instrument_functions_include_functions, arg);
>  +  break;




https://reviews.llvm.org/D37624



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

[PATCH] D31417: [OpenMP] Add support for omp simd pragmas without runtime

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D31417#886826, @huntergr wrote:

> In https://reviews.llvm.org/D31417#886441, @hfinkel wrote:
>
> > Is this still being worked on?
>
>
> Hi, yes it is. Sorry for the delay in posting new changes but priorities 
> shifted a bit and I had to work on something else for a while.


No problem.

> I do have a new version that's just about ready based on Alexey's idea. Simd 
> constructs work, and non-simd constructs just pass the code straight through; 
> combined constructs pose a bit of an issue, since they will have captured 
> stmts from non-simd directives as well. Some additional work will be needed 
> to handle that case. I'll post what I have after a bit of tidying for 
> comments and suggestions; it's possible that the mixed case could be dealt 
> with in another patch.

Splitting patches in this way is generally a good idea.

The other thing I'd like in this regard is some kind of preprocessor feature 
that my users can use to test whether OpenMP SIMD is supported. Enabling 
-fopenmp-simd won't define _OPENMP (at least I suspect that it shouldn't 
because there might not be a meaningful  to include).  Maybe we should 
make `#if __has_extension(openmp_simd)` work?


https://reviews.llvm.org/D31417



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

[PATCH] D38372: [OpenMP] Fix passing of -m arguments correctly

[Hal Finkel via Phabricator via cfe-commits]

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

LGTM


https://reviews.llvm.org/D38372



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

[PATCH] D32199: [TySan] A Type Sanitizer (Clang)

[Hal Finkel via Phabricator via cfe-commits]

hfinkel updated this revision to Diff 117619.
hfinkel added a comment.
Herald added a subscriber: kosarev.

Rebased.


https://reviews.llvm.org/D32199

Files:
  include/clang/Basic/Sanitizers.def
  include/clang/Driver/SanitizerArgs.h
  lib/CodeGen/BackendUtil.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CGDeclCXX.cpp
  lib/CodeGen/CodeGenFunction.cpp
  lib/CodeGen/CodeGenModule.cpp
  lib/CodeGen/CodeGenTBAA.cpp
  lib/CodeGen/SanitizerMetadata.cpp
  lib/CodeGen/SanitizerMetadata.h
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChains/CommonArgs.cpp
  lib/Driver/ToolChains/Linux.cpp
  lib/Lex/PPMacroExpansion.cpp
  test/CodeGen/sanitize-type-attr.cpp
  test/Driver/sanitizer-ld.c

Index: test/Driver/sanitizer-ld.c
===
--- test/Driver/sanitizer-ld.c
+++ test/Driver/sanitizer-ld.c
@@ -193,6 +193,18 @@
 
 // RUN: %clangxx -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: -target x86_64-unknown-linux -fuse-ld=ld -stdlib=platform -lstdc++ \
+// RUN: -fsanitize=type \
+// RUN: -resource-dir=%S/Inputs/resource_dir \
+// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-TYSAN-LINUX-CXX %s
+//
+// CHECK-TYSAN-LINUX-CXX: "{{(.*[^-.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
+// CHECK-TYSAN-LINUX-CXX-NOT: stdc++
+// CHECK-TYSAN-LINUX-CXX: "-whole-archive" "{{.*}}libclang_rt.tysan-x86_64.a" "-no-whole-archive"
+// CHECK-TYSAN-LINUX-CXX: stdc++
+
+// RUN: %clangxx -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN: -target x86_64-unknown-linux -fuse-ld=ld -stdlib=platform -lstdc++ \
 // RUN: -fsanitize=memory \
 // RUN: -resource-dir=%S/Inputs/resource_dir \
 // RUN: --sysroot=%S/Inputs/basic_linux_tree \
Index: test/CodeGen/sanitize-type-attr.cpp
===
--- /dev/null
+++ test/CodeGen/sanitize-type-attr.cpp
@@ -0,0 +1,75 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - %s | FileCheck -check-prefix=WITHOUT %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - %s -fsanitize=type | FileCheck -check-prefix=TYSAN %s
+// RUN: echo "src:%s" | sed -e 's/\\//g' > %t
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - %s -fsanitize=type -fsanitize-blacklist=%t | FileCheck -check-prefix=BL %s
+
+// The sanitize_type attribute should be attached to functions
+// when TypeSanitizer is enabled, unless no_sanitize("type") attribute
+// is present.
+
+// WITHOUT:  NoTYSAN1{{.*}}) [[NOATTR:#[0-9]+]]
+// BL:  NoTYSAN1{{.*}}) [[NOATTR:#[0-9]+]]
+// TYSAN:  NoTYSAN1{{.*}}) [[NOATTR:#[0-9]+]]
+__attribute__((no_sanitize("type")))
+int NoTYSAN1(int *a) { return *a; }
+
+// WITHOUT:  NoTYSAN2{{.*}}) [[NOATTR]]
+// BL:  NoTYSAN2{{.*}}) [[NOATTR]]
+// TYSAN:  NoTYSAN2{{.*}}) [[NOATTR]]
+__attribute__((no_sanitize("type")))
+int NoTYSAN2(int *a);
+int NoTYSAN2(int *a) { return *a; }
+
+// WITHOUT:  NoTYSAN3{{.*}}) [[NOATTR:#[0-9]+]]
+// BL:  NoTYSAN3{{.*}}) [[NOATTR:#[0-9]+]]
+// TYSAN:  NoTYSAN3{{.*}}) [[NOATTR:#[0-9]+]]
+__attribute__((no_sanitize("type")))
+int NoTYSAN3(int *a) { return *a; }
+
+// WITHOUT:  TYSANOk{{.*}}) [[NOATTR]]
+// BL:  TYSANOk{{.*}}) [[NOATTR]]
+// TYSAN: TYSANOk{{.*}}) [[WITH:#[0-9]+]]
+int TYSANOk(int *a) { return *a; }
+
+// WITHOUT:  TemplateTYSANOk{{.*}}) [[NOATTR]]
+// BL:  TemplateTYSANOk{{.*}}) [[NOATTR]]
+// TYSAN: TemplateTYSANOk{{.*}}) [[WITH]]
+template
+int TemplateTYSANOk() { return i; }
+
+// WITHOUT:  TemplateNoTYSAN{{.*}}) [[NOATTR]]
+// BL:  TemplateNoTYSAN{{.*}}) [[NOATTR]]
+// TYSAN: TemplateNoTYSAN{{.*}}) [[NOATTR]]
+template
+__attribute__((no_sanitize("type")))
+int TemplateNoTYSAN() { return i; }
+
+int force_instance = TemplateTYSANOk<42>()
+   + TemplateNoTYSAN<42>();
+
+// Check that __cxx_global_var_init* get the sanitize_type attribute.
+int global1 = 0;
+int global2 = *(int*)((char*)&global1+1);
+// WITHOUT: @__cxx_global_var_init{{.*}}[[NOATTR:#[0-9]+]]
+// BL: @__cxx_global_var_init{{.*}}[[NOATTR:#[0-9]+]]
+// TYSAN: @__cxx_global_var_init{{.*}}[[WITH:#[0-9]+]]
+
+// Make sure that we don't add globals to the list for which we don't have a
+// specific type description.
+struct SX { int a, b; };
+SX sx;
+
+// WITHOUT: attributes [[NOATTR]] = { noinline nounwind{{.*}} }
+
+// BL: attributes [[NOATTR]] = { noinline nounwind{{.*}} }
+
+// TYSAN: attributes [[NOATTR]] = { noinline nounwind{{.*}} }
+// TYSAN: attributes [[WITH]] = { noinline nounwind sanitize_type{{.*}} }
+
+// TYSAN-DAG: !llvm.tysan.globals = !{[[G1MD:![0-9]+]], [[G2MD:![0-9]+]], [[G3MD:![0-9]+]]}
+// TYSAN-DAG: [[G1MD]] = !{i32* @force_instance, [[INTMD:![0-9]+]]}
+// TYSAN-DAG: [[INTMD]] = !{!"int",
+// TYSAN-DAG: [[G2MD]] = !{i32* @global1, [[INTMD]]}
+// TYSAN-DAG: [[G3MD]] = !{i32* @global2, [[INTMD]]}
+// TYSAN-DAG: Simple C++ TBAA
+
Index: lib/Lex/PPMacroExpansion.cpp
===
--- lib/Lex/PPMacroExpansion.cpp
+++ lib/Lex/PP

[PATCH] D38596: Implement attribute target multiversioning

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

Can you explain briefly what is required of the system in order to support this 
(is it just ifunc support in the dynamic linker / loader)?

In general, there are a number of places in this patch, at the Sema layer 
(including in the error messages), that talk about how this is an x86-only 
feature. As soon as someone adds support for a second architecture, this will 
cause unnecessary churn (and perhaps miss places resulting in stale comments). 
Can you create some callbacks, in TargetInfo or similar, so that we can just 
naturally make this work on other architectures?

> Function templates are NOT supported (not supported in GCC either).

Can you please elaborate on this? I'd like to see this feature, but I don't 
believe that we should introduce features that artificially force users to 
choose between good code design and technical capabilities. The fact that GCC 
does not support this feature on templates seems unfortunate, but not in itself 
a justification to replicate that restriction. I'm obviously fine with adding 
template functions in a follow-up commit, but I want to understand whether 
there's something in the design (of the feature or of your implementation 
approach) that makes this hard. I wouldn't want this to ship without template 
support.




Comment at: lib/Basic/Targets/X86.cpp:1330
+  // ordering of the GCC Target attribute.
+  const auto TargetArray = {"avx512vpopcntdq",
+"avx5124fmaps",

How are we expected to maintain this array?


https://reviews.llvm.org/D38596



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

[PATCH] D38596: Implement attribute target multiversioning

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D38596#889697, @erichkeane wrote:

> In https://reviews.llvm.org/D38596#889682, @hfinkel wrote:
>
> > Can you explain briefly what is required of the system in order to support 
> > this (is it just ifunc support in the dynamic linker / loader)?
>
>
> Yep, this feature depends entirely on ifuncs.
>
> > In general, there are a number of places in this patch, at the Sema layer 
> > (including in the error messages), that talk about how this is an x86-only 
> > feature. As soon as someone adds support for a second architecture, this 
> > will cause unnecessary churn (and perhaps miss places resulting in stale 
> > comments). Can you create some callbacks, in TargetInfo or similar, so that 
> > we can just naturally make this work on other architectures?
>
> Yep, I have that enforced, with the hope that it would make it easier to 
> identify all the places that this change needs to be made.  Currently, there 
> are a few 'TargetInfo' features that need to be supported 
> (isValidCPUName/isValidCPUFeature/compareCpusAndFeatures), the SEMA check 
> relaxed (though I could perhaps add a 
> TargetInfo::IsFunctionMultiVersioningSupported function if thats what you're 
> saying?), and 
> CodeGenFunction::FormResolverCondition/CodeGenFunction::EmitMultiVersionResolver
>  ( which could easily have FormResolverCondition moved to TargetInfo, 
> assuming we OK with a CodeGen file calling into a TargetInfo?).  Are these 
> the changes you mean?


I added some comments inline.

> 
> 
>> 
>> 
>>> Function templates are NOT supported (not supported in GCC either).
>> 
>> Can you please elaborate on this? I'd like to see this feature, but I don't 
>> believe that we should introduce features that artificially force users to 
>> choose between good code design and technical capabilities. The fact that 
>> GCC does not support this feature on templates seems unfortunate, but not in 
>> itself a justification to replicate that restriction. I'm obviously fine 
>> with adding template functions in a follow-up commit, but I want to 
>> understand whether there's something in the design (of the feature or of 
>> your implementation approach) that makes this hard. I wouldn't want this to 
>> ship without template support.
> 
> I don't think there are limitations beyond my understanding of the code 
> around redefinitions of templates.  I investigated for a while and was unable 
> to figure it out over a few days, but that doesn't mean terribly much.  I 
> gave up temporarily in exchange for getting the rest of the feature in (and 
> the fact that GCC doesn't support it made it an explainable stopping point; 
> additionally, none of our users of 'attribute-target' actually have C++ 
> codebases).

I anticipate this will change.

>   I believe I'd be able to add function-template support with some additional 
> work, and am definitely willing to do so.

Great, thanks!

> Thanks Hal, I appreciate your advice/feedback!  I realize this is a huge 
> patch, so anything I can get to make this more acceptable is greatly 
> appreciated.






Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9305
+: Error<"function multiversioning with 'target' is only supported on "
+"X86/x86_64 architectures">;
+def err_bad_multiversion_option

I'd not mention x86 here. As soon as we add support for another architecture, 
we'll need to reword this anyway.

  : Error<"function multiversioning with 'target' is not supported on this 
architecture and platform">;




Comment at: lib/Basic/Targets/X86.cpp:1295
   .Case("amdfam10h", true)
+  .Case("amdfam10", true)
   .Case("amdfam15h", true)

Can you please separate out these changes adding the amdfam10 target strings 
into a separate patch?



Comment at: lib/Basic/Targets/X86.cpp:1330
+  // ordering of the GCC Target attribute.
+  const auto TargetArray = {"avx512vpopcntdq",
+"avx5124fmaps",

erichkeane wrote:
> hfinkel wrote:
> > How are we expected to maintain this array?
> Ugg... I know.  I have no idea, it is ANOTHER implementation of this foolish 
> list, just in a slightly different order.  We NEED to know the order, so we 
> know how to order the comparisons.  They are generally arbitrary orderings, 
> so I have no idea besides "with hard work".  If you have a suggestion, or 
> perhaps a way to reorder one of the OTHER lists, I'd love to hear it.
At the risk of a comment that will become stale at some point, is it possible 
to say something here like:

  // This list has to match the list in GCC. In GCC 6.whatever, the list is in 
path/to/file/in/gcc.c

At least that would give someone a clue of how to update this list if necessary.




Comment at: lib/Sema/SemaDecl.cpp:9249
+
+  // Need to check isValidCPUName since it checks X86 vs X86-64 CPUs.  From
+  // there, the subset of va

[PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added inline comments.



Comment at: lib/CodeGen/CGRecordLayoutBuilder.cpp:411
+  // component so as it can be accessed directly with lower cost.
+  auto betterBeSingleFieldRun = [&](RecordDecl::field_iterator Field) {
+if (!Types.getCodeGenOpts().FineGrainedBitfieldAccesses)

betterBeSingleFieldRun -> IsBetterAsSingleFieldRun



Comment at: lib/CodeGen/CGRecordLayoutBuilder.cpp:444
 // Add bitfields to the run as long as they qualify.
-if (Field != FieldEnd && Field->getBitWidthValue(Context) != 0 &&
-Tail == getFieldBitOffset(*Field)) {
-  Tail += Field->getBitWidthValue(Context);
-  ++Field;
-  continue;
+if (Field != FieldEnd && !SingleFieldRun) {
+  SingleFieldRun = betterBeSingleFieldRun(Field);

The logic here is not obvious. Can you please add a comment. SingleFieldRun 
here is only not equal to `betterBeSingleFieldRun(Field)` if we've skipped 
0-length bitfields, right? Please explain what's going on and also please make 
sure there's a test case.


Repository:
  rL LLVM

https://reviews.llvm.org/D36562



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

[PATCH] D24933: Enable configuration files in clang

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

Some suggested improvements to the English in the documentation...




Comment at: docs/UsersManual.rst:700
+
+Configuration files group command line options and allow to specify all of
+them just by referencing the configuration file. They may be used, for

Configuration files group command-line options and allow all of them to be 
specified just by ...



Comment at: docs/UsersManual.rst:702
+them just by referencing the configuration file. They may be used, for
+instance, to collect options required to tune compilation for particular
+target, such as -L, -I, -l, --sysroot, codegen options etc.

instance -> example

"for instance" and "for example" are essentially the same, but for consistency, 
"for example" is much more common in this document, so we should standardize.



Comment at: docs/UsersManual.rst:703
+instance, to collect options required to tune compilation for particular
+target, such as -L, -I, -l, --sysroot, codegen options etc.
+

options, etc.



Comment at: docs/UsersManual.rst:706
+The command line option `--config` can be used to specify configuration
+file in a Clang invocation. For instance:
+

instance -> example



Comment at: docs/UsersManual.rst:714
+If the provided argument contains a directory separator, it is considered as
+a file path, options are read from that file. Otherwise the argument is treated
+as a file name and is searched for sequentially in the directories:

and options are



Comment at: docs/UsersManual.rst:719
+- the directory where Clang executable resides.
+Both user and system directory for configuration files are specified during
+clang build using cmake parameters, CLANG_CONFIG_FILE_USER_DIR and

directory -> directories



Comment at: docs/UsersManual.rst:720
+Both user and system directory for configuration files are specified during
+clang build using cmake parameters, CLANG_CONFIG_FILE_USER_DIR and
+CLANG_CONFIG_FILE_SYSTEM_DIR respectively. The first found file is used. It is

cmake -> CMake



Comment at: docs/UsersManual.rst:721
+clang build using cmake parameters, CLANG_CONFIG_FILE_USER_DIR and
+CLANG_CONFIG_FILE_SYSTEM_DIR respectively. The first found file is used. It is
+an error if the required file cannot be found.

The first file found is used.



Comment at: docs/UsersManual.rst:724
+
+Another way to specify configuration file is to encode it in executable name. 
For
+instance, if Clang executable is named `armv7l-clang` (it may be a symbolic 
link

specify a configuration file



Comment at: docs/UsersManual.rst:725
+Another way to specify configuration file is to encode it in executable name. 
For
+instance, if Clang executable is named `armv7l-clang` (it may be a symbolic 
link
+to `clang`), then Clang will search file `armv7l.cfg` in the directory where 
Clang

if the Clang executable



Comment at: docs/UsersManual.rst:726
+instance, if Clang executable is named `armv7l-clang` (it may be a symbolic 
link
+to `clang`), then Clang will search file `armv7l.cfg` in the directory where 
Clang
+resides.

will search for file



Comment at: docs/UsersManual.rst:729
+
+If driver mode is specified in invocation, Clang tries to find file specific 
for
+the specified mode. For instance, if executable file is `x86_64-clang-cl`, 
Clang

If a driver mode



Comment at: docs/UsersManual.rst:729
+
+If driver mode is specified in invocation, Clang tries to find file specific 
for
+the specified mode. For instance, if executable file is `x86_64-clang-cl`, 
Clang

hfinkel wrote:
> If a driver mode
to find a file



Comment at: docs/UsersManual.rst:730
+If driver mode is specified in invocation, Clang tries to find file specific 
for
+the specified mode. For instance, if executable file is `x86_64-clang-cl`, 
Clang
+first looks for `x86_64-cl.cfg` and if it is not found, looks for `x86_64.cfg'.

if the executable is named `x86_64-clang-cl`



Comment at: docs/UsersManual.rst:733
+
+If command line contains options that effectively changes target architecture
+(these are -m32, -EL and some other) and configuration file starts with 
architecture

If the command line



Comment at: docs/UsersManual.rst:733
+
+If command line contains options that effectively changes target architecture
+(these are -m32, -EL and some other) and configuration file starts with 
architecture

hfinkel wrote:
> If the command line
changes -> change



Comment at: docs/UsersManual.rst:734
+If command line contains options that effecti

[PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added inline comments.



Comment at: include/clang/Basic/DiagnosticDriverKinds.td:335
+def warn_drv_fine_grained_bitfield_accesses_ignored : Warning<
+  "option '-ffine-grained-bitfield-accesses' cannot be enabled together with 
sanitizer; flag ignored">,
+  InGroup;

with a sanitizer



Comment at: include/clang/Driver/Options.td:1041
+  "ffine-grained-bitfield-accesses">, Group, Flags<[CC1Option]>,
+  HelpText<"Use separate access for bitfields with legal widths and 
alignments.">;
+def fno_fine_grained_bitfield_accesses : Flag<["-"],

access -> accesses



Comment at: include/clang/Driver/Options.td:1044
+  "fno-fine-grained-bitfield-accesses">, Group, 
Flags<[CC1Option]>,
+  HelpText<"Use a big integer wrap for a consecutive run of bitfields.">;
+

Use large-integer access for consecutive bitfield runs.



Comment at: include/clang/Frontend/CodeGenOptions.def:182
 CODEGENOPT(SoftFloat , 1, 0) ///< -soft-float.
+CODEGENOPT(FineGrainedBitfieldAccesses, 1, 0) ///< Use separate access for 
bitfields
+  ///< with legal widths and 
alignments.

These lines are too long.



Comment at: lib/CodeGen/CGRecordLayoutBuilder.cpp:449
+// Otherwise, try to add bitfields to the run.
+if (Run != FieldEnd && !IsBetterAsSingleFieldRun(Run) &&
+Field != FieldEnd && !IsBetterAsSingleFieldRun(Field) &&

Why do you have the `IsBetterAsSingleFieldRun(Run)` check here (where we'll 
evaluate it multiple times (for all of the fields in the run)). Can't you make 
the predicate above directly?

  // Any non-zero-length bitfield can start a new run.
  if (Field->getBitWidthValue(Context) != 0 &&
   !IsBetterAsSingleFieldRun(Field)) {
Run = Field;
StartBitOffset = getFieldBitOffset(*Field);
...



Repository:
  rL LLVM

https://reviews.llvm.org/D36562



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

[PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

[Hal Finkel via Phabricator via cfe-commits]

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

LGTM




Comment at: include/clang/Frontend/CodeGenOptions.def:182
 CODEGENOPT(SoftFloat , 1, 0) ///< -soft-float.
+CODEGENOPT(FineGrainedBitfieldAccesses, 1, 0) ///< Enable finegrained bitfield 
accesses.
 CODEGENOPT(StrictEnums   , 1, 0) ///< Optimize based on strict enum 
definition.

finegrained -> fine-grained

(I suppose we're not sticking to 80 cols in this file anyway)



Repository:
  rL LLVM

https://reviews.llvm.org/D36562



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

[PATCH] D32199: [TySan] A Type Sanitizer (Clang)

[Hal Finkel via Phabricator via cfe-commits]

hfinkel updated this revision to Diff 119103.
hfinkel added a comment.

Rebased.


https://reviews.llvm.org/D32199

Files:
  include/clang/Basic/Sanitizers.def
  include/clang/Driver/SanitizerArgs.h
  lib/CodeGen/BackendUtil.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CGDeclCXX.cpp
  lib/CodeGen/CodeGenFunction.cpp
  lib/CodeGen/CodeGenModule.cpp
  lib/CodeGen/CodeGenTBAA.cpp
  lib/CodeGen/SanitizerMetadata.cpp
  lib/CodeGen/SanitizerMetadata.h
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChains/CommonArgs.cpp
  lib/Driver/ToolChains/Linux.cpp
  lib/Lex/PPMacroExpansion.cpp
  test/CodeGen/sanitize-type-attr.cpp
  test/Driver/sanitizer-ld.c

Index: test/Driver/sanitizer-ld.c
===
--- test/Driver/sanitizer-ld.c
+++ test/Driver/sanitizer-ld.c
@@ -239,6 +239,18 @@
 
 // RUN: %clangxx -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: -target x86_64-unknown-linux -fuse-ld=ld -stdlib=platform -lstdc++ \
+// RUN: -fsanitize=type \
+// RUN: -resource-dir=%S/Inputs/resource_dir \
+// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-TYSAN-LINUX-CXX %s
+//
+// CHECK-TYSAN-LINUX-CXX: "{{(.*[^-.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
+// CHECK-TYSAN-LINUX-CXX-NOT: stdc++
+// CHECK-TYSAN-LINUX-CXX: "-whole-archive" "{{.*}}libclang_rt.tysan-x86_64.a" "-no-whole-archive"
+// CHECK-TYSAN-LINUX-CXX: stdc++
+
+// RUN: %clangxx -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN: -target x86_64-unknown-linux -fuse-ld=ld -stdlib=platform -lstdc++ \
 // RUN: -fsanitize=memory \
 // RUN: -resource-dir=%S/Inputs/resource_dir \
 // RUN: --sysroot=%S/Inputs/basic_linux_tree \
Index: test/CodeGen/sanitize-type-attr.cpp
===
--- /dev/null
+++ test/CodeGen/sanitize-type-attr.cpp
@@ -0,0 +1,75 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - %s | FileCheck -check-prefix=WITHOUT %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - %s -fsanitize=type | FileCheck -check-prefix=TYSAN %s
+// RUN: echo "src:%s" | sed -e 's/\\//g' > %t
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - %s -fsanitize=type -fsanitize-blacklist=%t | FileCheck -check-prefix=BL %s
+
+// The sanitize_type attribute should be attached to functions
+// when TypeSanitizer is enabled, unless no_sanitize("type") attribute
+// is present.
+
+// WITHOUT:  NoTYSAN1{{.*}}) [[NOATTR:#[0-9]+]]
+// BL:  NoTYSAN1{{.*}}) [[NOATTR:#[0-9]+]]
+// TYSAN:  NoTYSAN1{{.*}}) [[NOATTR:#[0-9]+]]
+__attribute__((no_sanitize("type")))
+int NoTYSAN1(int *a) { return *a; }
+
+// WITHOUT:  NoTYSAN2{{.*}}) [[NOATTR]]
+// BL:  NoTYSAN2{{.*}}) [[NOATTR]]
+// TYSAN:  NoTYSAN2{{.*}}) [[NOATTR]]
+__attribute__((no_sanitize("type")))
+int NoTYSAN2(int *a);
+int NoTYSAN2(int *a) { return *a; }
+
+// WITHOUT:  NoTYSAN3{{.*}}) [[NOATTR:#[0-9]+]]
+// BL:  NoTYSAN3{{.*}}) [[NOATTR:#[0-9]+]]
+// TYSAN:  NoTYSAN3{{.*}}) [[NOATTR:#[0-9]+]]
+__attribute__((no_sanitize("type")))
+int NoTYSAN3(int *a) { return *a; }
+
+// WITHOUT:  TYSANOk{{.*}}) [[NOATTR]]
+// BL:  TYSANOk{{.*}}) [[NOATTR]]
+// TYSAN: TYSANOk{{.*}}) [[WITH:#[0-9]+]]
+int TYSANOk(int *a) { return *a; }
+
+// WITHOUT:  TemplateTYSANOk{{.*}}) [[NOATTR]]
+// BL:  TemplateTYSANOk{{.*}}) [[NOATTR]]
+// TYSAN: TemplateTYSANOk{{.*}}) [[WITH]]
+template
+int TemplateTYSANOk() { return i; }
+
+// WITHOUT:  TemplateNoTYSAN{{.*}}) [[NOATTR]]
+// BL:  TemplateNoTYSAN{{.*}}) [[NOATTR]]
+// TYSAN: TemplateNoTYSAN{{.*}}) [[NOATTR]]
+template
+__attribute__((no_sanitize("type")))
+int TemplateNoTYSAN() { return i; }
+
+int force_instance = TemplateTYSANOk<42>()
+   + TemplateNoTYSAN<42>();
+
+// Check that __cxx_global_var_init* get the sanitize_type attribute.
+int global1 = 0;
+int global2 = *(int*)((char*)&global1+1);
+// WITHOUT: @__cxx_global_var_init{{.*}}[[NOATTR:#[0-9]+]]
+// BL: @__cxx_global_var_init{{.*}}[[NOATTR:#[0-9]+]]
+// TYSAN: @__cxx_global_var_init{{.*}}[[WITH:#[0-9]+]]
+
+// Make sure that we don't add globals to the list for which we don't have a
+// specific type description.
+struct SX { int a, b; };
+SX sx;
+
+// WITHOUT: attributes [[NOATTR]] = { noinline nounwind{{.*}} }
+
+// BL: attributes [[NOATTR]] = { noinline nounwind{{.*}} }
+
+// TYSAN: attributes [[NOATTR]] = { noinline nounwind{{.*}} }
+// TYSAN: attributes [[WITH]] = { noinline nounwind sanitize_type{{.*}} }
+
+// TYSAN-DAG: !llvm.tysan.globals = !{[[G1MD:![0-9]+]], [[G2MD:![0-9]+]], [[G3MD:![0-9]+]]}
+// TYSAN-DAG: [[G1MD]] = !{i32* @force_instance, [[INTMD:![0-9]+]]}
+// TYSAN-DAG: [[INTMD]] = !{!"int",
+// TYSAN-DAG: [[G2MD]] = !{i32* @global1, [[INTMD]]}
+// TYSAN-DAG: [[G3MD]] = !{i32* @global2, [[INTMD]]}
+// TYSAN-DAG: Simple C++ TBAA
+
Index: lib/Lex/PPMacroExpansion.cpp
===
--- lib/Lex/PPMacroExpansion.cpp
+++ lib/Lex/PPMacroExpansion.cpp
@@ -1138,6 +1138,

[PATCH] D39083: [CodeGen] Fix generation of TBAA info for array-to-pointer conversions

[Hal Finkel via Phabricator via cfe-commits]

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

LGTM


Repository:
  rL LLVM

https://reviews.llvm.org/D39083



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

[PATCH] D39160: [CodeGen] __builtin_sqrt should map to the compiler's intrinsic sqrt function

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D39160#902918, @spatel wrote:

> Oops - I wrongly made llvm-commits a subscriber rather than cfe-commits. Let 
> me know if I should reopen under a new thread to get the patch to hit the 
> right mailing list.


Yes, please reopen.


https://reviews.llvm.org/D39160



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

[PATCH] D39053: [Bitfield] Add more cases to making the bitfield a separate location

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

> With this patch we avoid unaligned loads and stores, at least on MIPS 
> architecture.

Can you please explain the nature of the problematic situations?

Also, this patch does not update the comments that describe the splitting 
algorithm. That should be improved.

If this is just addressing the classic problem that, once we narrow a load we 
can't re-widen it, it might be best to solve this another way (e.g., by adding 
metadata noting the dereferenceable range).


https://reviews.llvm.org/D39053



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

[PATCH] D39005: [OpenMP] Clean up variable and function names for NVPTX backend

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D39005#900973, @jlebar wrote:

> > The first question that comes to mind is what is the link between data 
> > layout and name mangling conventions?
>
> I pulled up http://llvm.org/doxygen/classllvm_1_1DataLayout.html and searched 
> for "mangling" -- presumably this is what they were referring to.  We also 
> don't need to speculate, rnk still works on LLVM.  :)


DataLayout generally holds information that the target-independent optimizer 
needs in order to simplify the IR into our canonical form. This is as opposed 
to TargetTransformInfo, which provides data necessary to optimize the IR in 
target-aware ways (e.g., do things that are orthogonal to canonicalization such 
as inlining and vectorization). It is also as opposed to external utility 
functions that might be used by the frontend (e.g., 
llvm::sys::getHostCPUName()). If I recall correctly, this is information that 
would be used by the frontend when generating the IR, and the function results 
are controlled by the triple. As a result, I think that a general utility 
function somewhere would be fine.


Repository:
  rL LLVM

https://reviews.llvm.org/D39005



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

[PATCH] D39138: [CodeGen] Generate TBAA info for 'this' pointers

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D39138#904747, @rjmccall wrote:

> Okay, if this is just for your own checking, I'd rather not take it.  It's 
> not a significant compile-time cost, but there's no reason to pay it at all.


In that case, can we take it? I'd rather have everything decorated for use by 
the type sanitizer.


Repository:
  rL LLVM

https://reviews.llvm.org/D39138



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

[PATCH] D39079: New clang option -fno-plt to avoid PLT for external calls

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added subscribers: vit9696, hfinkel.
hfinkel added a comment.

Noting that, as @vit9696  pointed out in https://reviews.llvm.org/D38554, this 
does not suppress uses of the PLT that occur from backend/optimizer-generated 
functions (e.g., calls into compiler-rt and similar).


https://reviews.llvm.org/D39079



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

[PATCH] D39079: New clang option -fno-plt to avoid PLT for external calls

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D39079#905371, @tmsriram wrote:

> In https://reviews.llvm.org/D39079#905353, @hfinkel wrote:
>
> > Noting that, as @vit9696  pointed out in https://reviews.llvm.org/D38554, 
> > this does not suppress uses of the PLT that occur from 
> > backend/optimizer-generated functions (e.g., calls into compiler-rt and 
> > similar).
>
>
> Can I work on this as a follow-up?


I have no objection to that, although adding a mechanism to fix this (which I 
imagine would be an attribute tied to the caller, not the callee, would 
probably end up replacing this mechanism).


https://reviews.llvm.org/D39079



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

[PATCH] D39079: New clang option -fno-plt to avoid PLT for external calls

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

> what is breaking the ELF interposition rules

Frankly, in retrospect, ELF's interposition rules (or at least the defaults for 
those rules), seem suboptimal on several fronts. While breaking them has some 
unfortunate consistency implications, I don't consider breaking them a bad 
thing.


https://reviews.llvm.org/D39079



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

[PATCH] D39138: [CodeGen] Generate TBAA info for 'this' pointers

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D39138#905445, @rjmccall wrote:

> In https://reviews.llvm.org/D39138#905184, @hfinkel wrote:
>
> > In https://reviews.llvm.org/D39138#904747, @rjmccall wrote:
> >
> > > Okay, if this is just for your own checking, I'd rather not take it.  
> > > It's not a significant compile-time cost, but there's no reason to pay it 
> > > at all.
> >
> >
> > In that case, can we take it? I'd rather have everything decorated for use 
> > by the type sanitizer.
>
>
> There will never, ever be an illegal access to this alloca.


Never mind, then ;)


Repository:
  rL LLVM

https://reviews.llvm.org/D39138



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

[PATCH] D37624: add support for -fno-instrument-functions and -finstrument-functions-exclude-{file, function}-list= to match gcc options.

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D37624#1211748, @apazos wrote:

> Hello folks, is there a plan to merge this feature still?


There's still a desire for the underlying functionality. We still need, as I 
recall, to figure out what's supposed to happen for C++ functions.


https://reviews.llvm.org/D37624



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

[PATCH] D47849: [OpenMP][Clang][NVPTX] Enable math functions called in an OpenMP NVPTX target device region to be resolved as device-native function calls

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D47849#1183134, @Hahnfeld wrote:

> In https://reviews.llvm.org/D47849#1124861, @hfinkel wrote:
>
> > In https://reviews.llvm.org/D47849#1124638, @Hahnfeld wrote:
> >
> > > 2. Incidentally I ran into a closely related problem: I can't `#include 
> > > ` in translation units compiled for offloading, Clang complains 
> > > about inline assembly for x86 (see below). Does that work for you?
> > >
> > >
> > >
> > >   In file included from /usr/include/math.h:413:
> > >   /usr/include/bits/mathinline.h:131:43: error: invalid input constraint 
> > > 'x' in asm
> > > __asm ("pmovmskb %1, %0" : "=r" (__m) : "x" (__x));
> > > ^
> > >   /usr/include/bits/mathinline.h:143:43: error: invalid input constraint 
> > > 'x' in asm
> > > __asm ("pmovmskb %1, %0" : "=r" (__m) : "x" (__x));
> > > ^
> > >   2 errors generated.
> >
> >
> > Hrmm. I thought that we had fixed that already.
> >
> > In case it's helpful, in an out-of-tree experimental target I have I ran 
> > into a similar problem, and to fix that I wrote the following code in the 
> > target's getTargetDefines function (in lib/Basic/Targets):
> >
> >   // If used as an OpenMP target on x86, x86 target feature macros are 
> > defined. math.h
> >   // and other system headers will include inline asm if these are defined.
> >   Builder.undefineMacro("__SSE2_MATH__");
> >   Builder.undefineMacro("__SSE_MATH__");
>
>
> Just found another workaround:
>
>   diff --git a/lib/Sema/SemaStmtAsm.cpp b/lib/Sema/SemaStmtAsm.cpp
>   index 0db15ea..b95f949 100644
>   --- a/lib/Sema/SemaStmtAsm.cpp
>   +++ b/lib/Sema/SemaStmtAsm.cpp
>   @@ -306,7 +306,9 @@ StmtResult Sema::ActOnGCCAsmStmt(SourceLocation AsmLoc, 
> bool IsSimple,
>   
>TargetInfo::ConstraintInfo Info(Literal->getString(), InputName);
>if 
> (!Context.getTargetInfo().validateInputConstraint(OutputConstraintInfos,
>   - Info)) {
>   + Info) &&
>   +!(Context.getLangOpts().OpenMPIsDevice &&
>   +  Context.getSourceManager().isInSystemHeader(AsmLoc))) {
>  return StmtError(Diag(Literal->getLocStart(),
>diag::err_asm_invalid_input_constraint)
>   << Info.getConstraintStr());
>
>
> This will ignore all errors during OpenMP device codegen from system headers 
> when the inline assembly is not used. In that case (calling `signbit`) you'll 
> get
>
>   In file included from math.c:2:
>   In file included from /usr/include/math.h:413:
>   /usr/include/bits/mathinline.h:143:10: error: couldn't allocate input reg 
> for constraint 'x'
> __asm ("pmovmskb %1, %0" : "=r" (__m) : "x" (__x));
>^
>   1 error generated.
>
>
> Not sure if that's acceptable...


Hrmm. Doesn't that make it so that whatever functions are implemented using 
that inline assembly will not be callable from target code (or, perhaps worse, 
will crash the backend if called)?


Repository:
  rC Clang

https://reviews.llvm.org/D47849



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

[PATCH] D47849: [OpenMP][Clang][NVPTX] Enable math functions called in an OpenMP NVPTX target device region to be resolved as device-native function calls

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D47849#1183996, @Hahnfeld wrote:

> In https://reviews.llvm.org/D47849#1183150, @hfinkel wrote:
>
> > Hrmm. Doesn't that make it so that whatever functions are implemented using 
> > that inline assembly will not be callable from target code (or, perhaps 
> > worse, will crash the backend if called)?
>
>
> You are right :-(
>
> However I'm getting worried about a more general case, not all inline 
> assembly is guarded by `#ifdef`s that we could hope to get right. For example 
> take `sys/io.h` which currently throws 18 errors when compiling with 
> offloading to GPUs, even with `-O0`. The inline assembly is only guarded by 
> `#if defined __GNUC__ && __GNUC__ >= 2` which should be defined by any modern 
> compiler claiming compatibility with GCC. I'm not sure this particular header 
> will ever end up in an OpenMP application, but others with inline assembly 
> will. From a quick grep it looks like some headers dealing with atomic 
> operations have inline assembly and even 
> `eigen3/Eigen/src/Core/util/Memory.h` for finding the cpuid.
>
> Coming back to the original problem: Maybe we need to undefine optimization 
> macros as in your patch to get as many correct inline functions as possible 
> AND ignore errors from inline assembly as in my patch to not break when 
> including weird headers?


The problem is that the inline assembly might actually be for the target, 
instead of the host, because we also have target preprocessor macros defined, 
and it's going to be hard to tell. I'm not sure that there's a great solution 
here, and I agree that having something more general than undefining some 
specific things that happen to matter for math.h would be better. As you point 
out, this is not just a system-header problem. We might indeed want to undefine 
all of the target-feature-related macros (although that won't always be 
sufficient, because we need basic arch macros for the system headers to work at 
all, and those are generally enough to guard some inline asm).

Maybe the following makes sense: Only define the host macros, minus 
target-feature ones, when compiling for the target in the context of the system 
headers. That makes the system headers work while providing a "clean" 
preprocessor environment for the rest of the code (and, thus, retains our 
ability to complain about bad inline asm).


Repository:
  rC Clang

https://reviews.llvm.org/D47849



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

[PATCH] D47849: [OpenMP][Clang][NVPTX] Enable math functions called in an OpenMP NVPTX target device region to be resolved as device-native function calls

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D47849#1184388, @Hahnfeld wrote:

> In https://reviews.llvm.org/D47849#1184367, @hfinkel wrote:
>
> > The problem is that the inline assembly might actually be for the target, 
> > instead of the host, because we also have target preprocessor macros 
> > defined, and it's going to be hard to tell. I'm not sure that there's a 
> > great solution here, and I agree that having something more general than 
> > undefining some specific things that happen to matter for math.h would be 
> > better. As you point out, this is not just a system-header problem. We 
> > might indeed want to undefine all of the target-feature-related macros 
> > (although that won't always be sufficient, because we need basic arch 
> > macros for the system headers to work at all, and those are generally 
> > enough to guard some inline asm).
>
>
> I think there was a reason for pulling in the host defines. I'd have to look 
> at the commit message though...


As I recall, it's mostly to make glibc's bits/wordsize.h work.

> 
> 
>> Maybe the following makes sense: Only define the host macros, minus 
>> target-feature ones, when compiling for the target in the context of the 
>> system headers. That makes the system headers work while providing a "clean" 
>> preprocessor environment for the rest of the code (and, thus, retains our 
>> ability to complain about bad inline asm).
> 
> I'm not sure how that's going to help with Eigen: Just including `Eigen/Core` 
> will pull in the other header file I mentioned with inline assembly. That's 
> completely independent of preprocessor macros, I think it's enough the 
> library's build system detected the host architecture during install.

I don't see any good way to satisfy Eigen in that form. I think that we'll need 
to update it to understand not to use host inline as when compiling for a 
target.


Repository:
  rC Clang

https://reviews.llvm.org/D47849



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

[PATCH] D50055: Update the coding standard about NFC changes and whitespace

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added inline comments.



Comment at: docs/CodingStandards.rst:512
 
+Do not commit changes that include trailing whitespace. Some common editors 
will
+automatically remove trailing whitespace when saving a file which causes

This statement is confusing (mostly because it has two reasonable 
interpretations and I think you actually mean both). We should say two separate 
things:

 1. As a coding guideline, make sure that lines don't have trailing whitespace.
 2. If such whitespace exists, don't remove it unless you're otherwise changing 
that line of code (and here we can caution people about their editors).




Comment at: docs/DeveloperPolicy.rst:395-408
+Commits with No Functional Change
+-
+
+It may be permissible to commit changes without prior review when the changes
+have no semantic impact on the code if the changes being made are obvious and
+not invasive. For instance, removing trailing whitespace from a line, fixing a
+line ending to be consistent with the rest of the file, fixing a typo, code

aaron.ballman wrote:
> chandlerc wrote:
> > I think this is a much broader statement than is warranted to address the 
> > specific problem. And I'm not completely comfortable with it.
> > 
> > I don't think guidance around "no functional change" is the right way to 
> > give guidance about what is or isn't "obvious" and fine to commit with 
> > post-commit review personally.
> > 
> > I'd really suggest just being direct about *formatting* and whitespace 
> > changes, and specifically suggest that they not be made unless the file or 
> > code region in question is an area that the author is planning subsequent 
> > changes to.
> We talk about formatting and whitespace in the CodingStandards document, but 
> we talk about obviousness and post-commit review in DeveloperPolicy. Where 
> would you like these new words to live? To me, they're more about the policy 
> and less about the coding standard -- the coding standard says what the code 
> should look like and the policy says what you should and should not do 
> procedurally, but then I feel the need to tie it back to "obviousness". How 
> about this in the developer policy:
> ```
> The Obviousness of Formatting Changes
> -
> 
> While formatting and whitespace changes may be "obvious", they can also create
> needless churn that causes difficulties for out-of-tree users carrying local
> patches. Do not commit formatting or whitespace changes unless they are 
> related
> to a file or code region that you intend to make subsequent changes to. The
> formatting and whitespace changes should be highly localized, committed before
> you begin functionality-changing work on the code region, and the commit 
> message
> should clearly state that the commit is not intended to change functionality,
> usually by stating it is :ref:`NFC `.
> 
> 
> If you wish to make a change to formatting or whitespace that touches an 
> entire
> library or code base, please seek pre-commit approval first.
> ```
I agree with @chandlerc about the current proposed text, and I think that this 
is better. I wonder if we want to insist on separating the commits, of if, 
combined localized commits are okay?



https://reviews.llvm.org/D50055



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

[PATCH] D48808: [CodeGen] Emit parallel_loop_access for each loop in the loop stack.

[Hal Finkel via Phabricator via cfe-commits]

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

LGTM


Repository:
  rC Clang

https://reviews.llvm.org/D48808



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

[PATCH] D48100: Append new attributes to the end of an AttributeList.

[Hal Finkel via Phabricator via cfe-commits]

hfinkel accepted this revision.
hfinkel added a comment.

Assuming that @aaron.ballman is still okay with this, I think that we should 
move forward. @erichkeane, I suspect that fixing the ordering problems, which 
were often arbitrary before and are still so, is a larger change that we'd want 
to split out regardless (not to mention the fact that we'd need to decide how 
to fix them). Erich, is that alright with you?


Repository:
  rC Clang

https://reviews.llvm.org/D48100



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

[PATCH] D42366: [CodeGen] Fix generation of TBAA tags for may-alias accesses

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D42366#1012211, @rjmccall wrote:

> I'm fine with that plan.  LGTM.


Thanks.

We should probably discuss what we want to do with unknown-sized things. 
UINT64_MAX is the conservative answer, but maybe there's some value in just 
having 0 mean "unknown size" so we don't need to encode a bunch of UINT64_MAX 
values at the IR level.


Repository:
  rC Clang

https://reviews.llvm.org/D42366



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

[PATCH] D42366: [CodeGen] Fix generation of TBAA tags for may-alias accesses

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D42366#1014546, @rjmccall wrote:

> In https://reviews.llvm.org/D42366#1014157, @kosarev wrote:
>
> > I think zero would serve better as the unknown-size value. People who are 
> > not aware of TBAA internals would guess that since zero-sized accesses make 
> > no sense, they are likely to have some special meaning. Similarly, for code 
> > that is supposed to process the size fields of access descriptors zero 
> > would be an obvious "illegal size value". In contrast, UINT64_MAX is just a 
> > very large number that doesn't hint anything on its special purpose.
>
>
> My thoughts exactly.
>
> John.


SGTM. Let's do that.


Repository:
  rC Clang

https://reviews.llvm.org/D42366



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

[PATCH] D33904: Add a __has_attribute_enhancement macro to clang

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

I prefer `__has_attribute_feature` to enhancement. I don't see why we need a 
new macro for this, however. Why not just use 
`__has_feature(overloadable_unmarked)` or similar?


https://reviews.llvm.org/D33904



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

[PATCH] D34357: [Clang] Handle interaction of -pg and no_instrument_function attribute.

[Hal Finkel via Phabricator via cfe-commits]

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

LGTM


https://reviews.llvm.org/D34357



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

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

[Hal Finkel via Phabricator via cfe-commits]

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

LGTM


Repository:
  rL LLVM

https://reviews.llvm.org/D29654



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

[PATCH] D29647: [OpenMP] Extend CLANG target options with device offloading kind.

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In general, this patch seems to be missing tests (unless it is actually NFC, or 
you can't write tests yet, which, in either case, need to be explained).




Comment at: lib/Driver/ToolChains/Cuda.cpp:217
+  std::vector GPUArchNames;
+  // If this is an OpenMP action we need to extract the device architecture 
from
+  // the -march option.

Do we have a test case for this?



Comment at: lib/Driver/ToolChains/Cuda.cpp:435
+
+// TODO: get the compute capability from offloading arguments when not
+// using the default compute capability of sm_20.

Why is this a TODO? Is the necessary information not currently available in the 
offloading arguments, or are we just not grabbing it in this patch?



Repository:
  rL LLVM

https://reviews.llvm.org/D29647



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

[PATCH] D29651: [OpenMP] Consider LIBRARY_PATH when selecting library paths for NVPTX targets in OpenMP mode.

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D29651#720179, @tstellar wrote:

> Why is this necessary?


To put it another way, what is special about LIBRARY_PATH in this regard? Do we 
already pass paths specified with `-L`? I'm also slightly concerned here with 
the custom parsing code. Can the logic from tools::addDirectoryList not be 
reused here?




Comment at: lib/Driver/ToolChains/Cuda.cpp:405
+
+  if (DeviceOffloadingKind == Action::OFK_OpenMP) {
+SmallVector LibraryPaths;

A comment is needed here explaining why this is necessary/desirable.



Comment at: lib/Driver/ToolChains/Cuda.cpp:412
+CompilerPath.split(llvm::sys::EnvPathSeparator);
+LibraryPaths.push_back(Split.first);
+CompilerPath = Split.second;

Is this the only use of this variable? How is this not dead code?



Comment at: test/Driver/openmp-offload.c:626
+// CHK-LIB-PATH: ld{{.*}}.o" "-L/a/b/c/lib"
+// CHK-LIB-PATH: ld{{.*}}.o" "-L/a/b/c/lib"

Can this be a little more specific so that you actually make sure you're 
getting the offloading linking step here?



Repository:
  rL LLVM

https://reviews.llvm.org/D29651



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

[PATCH] D39694: [VerifyDiagnosticConsumer] support -verify=

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added inline comments.



Comment at: lib/Frontend/VerifyDiagnosticConsumer.cpp:398
+// DToken is foo-bar-warning, but foo is the only -verify prefix).
+if (Prefixes.end() == std::find(Prefixes.begin(), Prefixes.end(), DToken))
+  continue;

> Converts from std::vector to std::set for more efficient prefix lookup.

Don't do that. First, std::find is O(N) here anyway. You'd want to use 
Prefixes.find(...) instead. However, this set is essentially constant and 
you're trying to optimize the lookup. In such a case, std::set is not a good 
choice. Just use a sorted vector instead (e.g., call std::sort on the vector), 
and then use std::binary_search here. That's likely much faster.



https://reviews.llvm.org/D39694



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

[PATCH] D40451: [OpenMP] Add function attribute for triggering shared memory lowering in the LLVM backend

[Hal Finkel via Phabricator via cfe-commits]

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

LGTM




Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:937
 if (!CapturedVars.empty()) {
+  // There's somehting to share, add the attribute
+  CGF.CurFn->addFnAttr("has-nvptx-shared-depot");

somehting -> something, add a period at the end of the comment.


Repository:
  rL LLVM

https://reviews.llvm.org/D40451



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

[PATCH] D40299: [Complex] Don't use __div?c3 when building with fast-math.

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added inline comments.



Comment at: lib/CodeGen/CGExprComplex.cpp:773
 // supported imaginary types in addition to complex types.
-if (RHSi) {
+if (RHSi && !FMF.isFast()) {
   BinOpInfo LibCallOp = Op;

fhahn wrote:
> Would the following structure be slightly easier to read?
> 
> if (RHSi) {
>   if (FMF.isFast()) { simplify } else {libcall}
> }
I'd use CGF.getLangOpts().FastMath (instead of interrogating the implicit state 
stored in the IR builder).



Comment at: test/CodeGen/complex-math.c:134
+  // FASTMATH-NOT: @__divsc3
+  // FASTMATH: fdiv
+  // FASTMATH: fdiv

Please check the actual expansion here and below.


https://reviews.llvm.org/D40299



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

[PATCH] D24933: Enable configuration files in clang

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added inline comments.



Comment at: lib/Driver/Driver.cpp:641
+
+static std::string getAbsolutePath(StringRef P) {
+  if (P.empty())

Use llvm::sys::fs::make_absolute instead of this.


https://reviews.llvm.org/D24933



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

[PATCH] D40594: [InstCombine] miscompile of __builtin_fmod

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D40594#940571, @spatel wrote:

> Added a blurb to the LangRef with https://reviews.llvm.org/rL319437, so I 
> think we can abandon this patch.


Sounds good.


https://reviews.llvm.org/D40594



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

[PATCH] D39694: [VerifyDiagnosticConsumer] support -verify=

[Hal Finkel via Phabricator via cfe-commits]

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

LGTM


https://reviews.llvm.org/D39694



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

[PATCH] D40995: [TextDiagnosticBuffer] Fix diagnostic note emission order.

[Hal Finkel via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC320904: [TextDiagnosticBuffer] Fix diagnostic note emission 
order (authored by hfinkel, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D40995

Files:
  include/clang/Frontend/TextDiagnosticBuffer.h
  lib/Frontend/TextDiagnosticBuffer.cpp
  test/Frontend/diagnostics-order.c


Index: include/clang/Frontend/TextDiagnosticBuffer.h
===
--- include/clang/Frontend/TextDiagnosticBuffer.h
+++ include/clang/Frontend/TextDiagnosticBuffer.h
@@ -29,6 +29,11 @@
   typedef DiagList::const_iterator const_iterator;
 private:
   DiagList Errors, Warnings, Remarks, Notes;
+  /// All - All diagnostics in the order in which they were generated.  That
+  /// order likely doesn't correspond to user input order, but it at least
+  /// keeps notes in the right places.  Each pair in the vector is a diagnostic
+  /// level and an index into the corresponding DiagList above.
+  std::vector> All;
 public:
   const_iterator err_begin() const  { return Errors.begin(); }
   const_iterator err_end() const{ return Errors.end(); }
Index: test/Frontend/diagnostics-order.c
===
--- test/Frontend/diagnostics-order.c
+++ test/Frontend/diagnostics-order.c
@@ -0,0 +1,10 @@
+// Make sure a note stays with its associated command-line argument diagnostic.
+// Previously, these diagnostics were grouped by diagnostic level with all
+// notes last.
+//
+// RUN: not %clang_cc1 -O999 -std=bogus %s 2> %t
+// RUN: FileCheck < %t %s
+//
+// CHECK: warning: optimization level '-O999' is not supported
+// CHECK-NEXT: error: invalid value 'bogus' in '-std=bogus'
+// CHECK-NEXT: note: use {{.*}} for {{.*}} standard
Index: lib/Frontend/TextDiagnosticBuffer.cpp
===
--- lib/Frontend/TextDiagnosticBuffer.cpp
+++ lib/Frontend/TextDiagnosticBuffer.cpp
@@ -30,34 +30,45 @@
   default: llvm_unreachable(
  "Diagnostic not handled during diagnostic 
buffering!");
   case DiagnosticsEngine::Note:
+All.emplace_back(Level, Notes.size());
 Notes.emplace_back(Info.getLocation(), Buf.str());
 break;
   case DiagnosticsEngine::Warning:
+All.emplace_back(Level, Warnings.size());
 Warnings.emplace_back(Info.getLocation(), Buf.str());
 break;
   case DiagnosticsEngine::Remark:
+All.emplace_back(Level, Remarks.size());
 Remarks.emplace_back(Info.getLocation(), Buf.str());
 break;
   case DiagnosticsEngine::Error:
   case DiagnosticsEngine::Fatal:
+All.emplace_back(Level, Errors.size());
 Errors.emplace_back(Info.getLocation(), Buf.str());
 break;
   }
 }
 
 void TextDiagnosticBuffer::FlushDiagnostics(DiagnosticsEngine &Diags) const {
-  // FIXME: Flush the diagnostics in order.
-  for (const_iterator it = err_begin(), ie = err_end(); it != ie; ++it)
-Diags.Report(Diags.getCustomDiagID(DiagnosticsEngine::Error, "%0"))
-<< it->second;
-  for (const_iterator it = warn_begin(), ie = warn_end(); it != ie; ++it)
-Diags.Report(Diags.getCustomDiagID(DiagnosticsEngine::Warning, "%0"))
-<< it->second;
-  for (const_iterator it = remark_begin(), ie = remark_end(); it != ie; ++it)
-Diags.Report(Diags.getCustomDiagID(DiagnosticsEngine::Remark, "%0"))
-<< it->second;
-  for (const_iterator it = note_begin(), ie = note_end(); it != ie; ++it)
-Diags.Report(Diags.getCustomDiagID(DiagnosticsEngine::Note, "%0"))
-<< it->second;
+  for (auto it = All.begin(), ie = All.end(); it != ie; ++it) {
+auto Diag = Diags.Report(Diags.getCustomDiagID(it->first, "%0"));
+switch (it->first) {
+default: llvm_unreachable(
+   "Diagnostic not handled during diagnostic 
flushing!");
+case DiagnosticsEngine::Note:
+  Diag << Notes[it->second].second;
+  break;
+case DiagnosticsEngine::Warning:
+  Diag << Warnings[it->second].second;
+  break;
+case DiagnosticsEngine::Remark:
+  Diag << Remarks[it->second].second;
+  break;
+case DiagnosticsEngine::Error:
+case DiagnosticsEngine::Fatal:
+  Diag << Errors[it->second].second;
+  break;
+}
+  }
 }
 


Index: include/clang/Frontend/TextDiagnosticBuffer.h
===
--- include/clang/Frontend/TextDiagnosticBuffer.h
+++ include/clang/Frontend/TextDiagnosticBuffer.h
@@ -29,6 +29,11 @@
   typedef DiagList::const_iterator const_iterator;
 private:
   DiagList Errors, Warnings, Remarks, Notes;
+  /// All - All diagnostics in the order in which they were generated.  That
+  /// order likely doesn't correspond to user input order, but it at least
+  /// keeps notes in the right places.  Each pair in the vector is a diagnostic
+  /// level and an index into the corresponding DiagList above.
+  std::vector> All;
 publi

[PATCH] D39694: [VerifyDiagnosticConsumer] support -verify=

[Hal Finkel via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC320908: [VerifyDiagnosticConsumer] support 
-verify= (authored by hfinkel, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D39694

Files:
  include/clang/Basic/DiagnosticDriverKinds.td
  include/clang/Basic/DiagnosticOptions.h
  include/clang/Driver/CC1Options.td
  lib/Frontend/CompilerInvocation.cpp
  lib/Frontend/VerifyDiagnosticConsumer.cpp
  test/Frontend/diagnostics-order.c
  test/Frontend/verify-prefixes.c
  test/Sema/tautological-unsigned-enum-zero-compare.c
  test/Sema/tautological-unsigned-enum-zero-compare.cpp
  test/Sema/tautological-unsigned-zero-compare.c

Index: include/clang/Basic/DiagnosticOptions.h
===
--- include/clang/Basic/DiagnosticOptions.h
+++ include/clang/Basic/DiagnosticOptions.h
@@ -100,6 +100,10 @@
   /// prefixes removed.
   std::vector Remarks;
 
+  /// The prefixes for comment directives sought by -verify ("expected" by
+  /// default).
+  std::vector VerifyPrefixes;
+
 public:
   // Define accessors/mutators for diagnostic options of enumeration type.
 #define DIAGOPT(Name, Bits, Default)
Index: include/clang/Basic/DiagnosticDriverKinds.td
===
--- include/clang/Basic/DiagnosticDriverKinds.td
+++ include/clang/Basic/DiagnosticDriverKinds.td
@@ -338,4 +338,8 @@
 def warn_drv_fine_grained_bitfield_accesses_ignored : Warning<
   "option '-ffine-grained-bitfield-accesses' cannot be enabled together with a sanitizer; flag ignored">,
   InGroup;
+
+def note_drv_verify_prefix_spelling : Note<
+  "-verify prefixes must start with a letter and contain only alphanumeric"
+  " characters, hyphens, and underscores">;
 }
Index: include/clang/Driver/CC1Options.td
===
--- include/clang/Driver/CC1Options.td
+++ include/clang/Driver/CC1Options.td
@@ -400,8 +400,12 @@
   HelpText<"Set the maximum number of source lines to show in a caret diagnostic">;
 def fmessage_length : Separate<["-"], "fmessage-length">, MetaVarName<"">,
   HelpText<"Format message diagnostics so that they fit within N columns or fewer, when possible.">;
+def verify_EQ : CommaJoined<["-"], "verify=">,
+  MetaVarName<"">,
+  HelpText<"Verify diagnostic output using comment directives that start with"
+   " prefixes in the comma-separated sequence ">;
 def verify : Flag<["-"], "verify">,
-  HelpText<"Verify diagnostic output using comment directives">;
+  HelpText<"Equivalent to -verify=expected">;
 def verify_ignore_unexpected : Flag<["-"], "verify-ignore-unexpected">,
   HelpText<"Ignore unexpected diagnostic messages">;
 def verify_ignore_unexpected_EQ : CommaJoined<["-"], "verify-ignore-unexpected=">,
Index: test/Sema/tautological-unsigned-enum-zero-compare.c
===
--- test/Sema/tautological-unsigned-enum-zero-compare.c
+++ test/Sema/tautological-unsigned-enum-zero-compare.c
@@ -1,6 +1,10 @@
-// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -DUNSIGNED -verify %s
-// RUN: %clang_cc1 -triple=x86_64-pc-win32 -fsyntax-only -DSIGNED -verify %s
-// RUN: %clang_cc1 -triple=x86_64-pc-win32 -fsyntax-only -Wno-tautological-unsigned-enum-zero-compare -verify %s
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only \
+// RUN:-verify=unsigned,unsigned-signed %s
+// RUN: %clang_cc1 -triple=x86_64-pc-win32 -fsyntax-only \
+// RUN:-verify=unsigned-signed %s
+// RUN: %clang_cc1 -triple=x86_64-pc-win32 -fsyntax-only \
+// RUN:-Wno-tautological-unsigned-enum-zero-compare \
+// RUN:-verify=silence %s
 
 // Okay, this is where it gets complicated.
 // Then default enum sigdness is target-specific.
@@ -12,175 +16,38 @@
   enum B { B_a = -1 };
   enum B b;
 
-#ifdef UNSIGNED
-  if (a < 0) // expected-warning {{comparison of unsigned enum expression < 0 is always false}}
-return 0;
-  if (0 >= a)
-return 0;
-  if (a > 0)
-return 0;
-  if (0 <= a) // expected-warning {{comparison of 0 <= unsigned enum expression is always true}}
-return 0;
-  if (a <= 0)
-return 0;
-  if (0 > a) // expected-warning {{comparison of 0 > unsigned enum expression is always false}}
-return 0;
-  if (a >= 0) // expected-warning {{comparison of unsigned enum expression >= 0 is always true}}
-return 0;
-  if (0 < a)
-return 0;
-
-  if (a < 0U) // expected-warning {{comparison of unsigned enum expression < 0 is always false}}
-return 0;
-  if (0U >= a)
-return 0;
-  if (a > 0U)
-return 0;
-  if (0U <= a) // expected-warning {{comparison of 0 <= unsigned enum expression is always true}}
-return 0;
-  if (a <= 0U)
-return 0;
-  if (0U > a) // expected-warning {{comparison of 0 > unsigned enum expression is always false}}
-return 0;
-  if (a >= 0U) // expected-warning {{compa

[PATCH] D41394: [CodeGen] Support generation of TBAA info in the new format

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D41394#959715, @rjmccall wrote:

> Rewriting some of the most basic tests would be fine.  Please either use new 
> FileCheck lines or clone the existing tests, since we don't really know how 
> long this transition will last.


+1

Otherwise, this looks fine to me.


Repository:
  rL LLVM

https://reviews.llvm.org/D41394



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

[PATCH] D41399: [CodeGen] Represent array members in new-format TBAA type descriptors

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added inline comments.



Comment at: test/CodeGen/tbaa-array.cpp:24
+// CHECK-DAG: [[TAG_A_i]] = !{[[TYPE_A:!.*]], [[TYPE_int:!.*]], i64 0, i64 4}
+// CHECK-DAG: [[TAG_C_i]] = !{[[TYPE_C:!.*]], [[TYPE_int:!.*]], i64 0, i64 16}
+// CHECK-DAG: [[TYPE_A]] = !{[[TYPE_char:!.*]], i64 4, !"_ZTS1A", 
[[TYPE_int]], i64 0, i64 4}

Shouldn't this access have a size of 4, and an access for c->x[2] have a size 
of 4 and a specific offset and c->x[j] have a size of 12 and an offset of zero? 
Why does this list a size of 16?

In any case, please add tests for:

  int *bar2(C *c) {
return c->x;
  }

  int bar3(C *c) {
return c->x[2];
  }

  int bar4(C *c, int j) {
return c->x[j];
  }



Repository:
  rL LLVM

https://reviews.llvm.org/D41399



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

[PATCH] D40299: [Complex] Don't use __div?c3 when building with fast-math.

[Hal Finkel via Phabricator via cfe-commits]

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

LGTM


https://reviews.llvm.org/D40299



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

[PATCH] D41399: [CodeGen] Represent array members in new-format TBAA type descriptors

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added inline comments.



Comment at: test/CodeGen/tbaa-array.cpp:24
+// CHECK-DAG: [[TAG_A_i]] = !{[[TYPE_A:!.*]], [[TYPE_int:!.*]], i64 0, i64 4}
+// CHECK-DAG: [[TAG_C_i]] = !{[[TYPE_C:!.*]], [[TYPE_int:!.*]], i64 0, i64 16}
+// CHECK-DAG: [[TYPE_A]] = !{[[TYPE_char:!.*]], i64 4, !"_ZTS1A", 
[[TYPE_int]], i64 0, i64 4}

kosarev wrote:
> kosarev wrote:
> > hfinkel wrote:
> > > Shouldn't this access have a size of 4, and an access for c->x[2] have a 
> > > size of 4 and a specific offset and c->x[j] have a size of 12 and an 
> > > offset of zero? Why does this list a size of 16?
> > > 
> > > In any case, please add tests for:
> > > 
> > >   int *bar2(C *c) {
> > > return c->x;
> > >   }
> > > 
> > >   int bar3(C *c) {
> > > return c->x[2];
> > >   }
> > > 
> > >   int bar4(C *c, int j) {
> > > return c->x[j];
> > >   }
> > > 
> > Indeed, the access size is wrong as we mistakenly inherit it from the base 
> > type. D41452 fixes this. Thanks for catching.
> Hal, in bar2() we don't really access memory. What do we want to check with 
> it?
> Hal, in bar2() we don't really access memory. What do we want to check with 
> it?

Ah, good point. Ignore that one.


Repository:
  rL LLVM

https://reviews.llvm.org/D41399



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

[PATCH] D40448: Add a printing policy for AST dumping

[Hal Finkel via Phabricator via cfe-commits]

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

In https://reviews.llvm.org/D40448#961398, @aaron.ballman wrote:

> P-p-p-power ping! :-)


LGTM




Comment at: lib/AST/ASTDumper.cpp:218
+: ASTDumper(OS, Traits, SM, ShowColors, LangOptions()) {}
+//ASTDumper(raw_ostream &OS, const CommandTraits *Traits,
+//  const SourceManager *SM, const PrintingPolicy &PrintPolicy)

Remove commented-out code.


https://reviews.llvm.org/D40448



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

[PATCH] D24933: Enable configuration files in clang

[Hal Finkel via Phabricator via cfe-commits]

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

LGTM




Comment at: lib/Driver/Driver.cpp:1508
+if (!SystemConfigDir.empty())
+  llvm::errs() << "System configuration file directory: "
+   << SystemConfigDir << "\n";

configuration file directory -> configuration-file directory



Comment at: lib/Driver/Driver.cpp:1511
+if (!UserConfigDir.empty())
+  llvm::errs() << "User configuration file directory: "
+   << UserConfigDir << "\n";

configuration file directory -> configuration-file directory


https://reviews.llvm.org/D24933



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

[PATCH] D41452: [CodeGen] Fix access sizes in new-format TBAA tags

[Hal Finkel via Phabricator via cfe-commits]

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

In https://reviews.llvm.org/D41452#961710, @rjmccall wrote:

> LGTM.


Me too.


Repository:
  rL LLVM

https://reviews.llvm.org/D41452



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

[PATCH] D41399: [CodeGen] Represent array members in new-format TBAA type descriptors

[Hal Finkel via Phabricator via cfe-commits]

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

LGTM. The array accesses here are just being represented as their scalar-access 
types. In the future, we should update this to represent them as fields in 
their parent structs, but this is fine for now.


https://reviews.llvm.org/D41399



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

[PATCH] D39457: [OPENMP] Current status of OpenMP support.

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D39457#961824, @Hahnfeld wrote:

> @hfinkel I think you requested this documentation on the mailing list. Can 
> you take a look if it matches your expectations so we can get this bundled in 
> the 6.0 release?


Yes, this looks good. Thank you.


Repository:
  rC Clang

https://reviews.llvm.org/D39457



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

[PATCH] D47267: [UnrollAndJam] Add unroll_and_jam pragma handling

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D47267#1123038, @Meinersbur wrote:

> In https://reviews.llvm.org/D47267#1123013, @dmgreen wrote:
>
> > I see your point about the mix of underscores. "nounroll_and_jam" also 
> > comes from the Intel docs, and theres already "nounroll" vs "unroll". The 
> > "no" becomes a qualifier on "unroll_and_jam". "no_unroll_and_jam" feels 
> > less consistent to me.
>
>
> `nounroll_and_jam` looks like it should be parsed as "(no unroll) and jam" 
> (do not unroll, but fuse) instead of "no (unroll-and-jam)" because `nounroll` 
> is one word and as you mentioned, already used as a keyword somewhere else. 
> Other variants use the underscore to append an option, e.g. `vectorize_width`.
>
> > But if you have a strong opinion, I'm happy enough to change it.
>
> I don't. Feel free to chose the name you think fits best. We might support 
> multiple spellings if necessary.


I agree that we can support multiple spellings (especially for compatibility 
with other compilers). I have a preference for using the underscores as our 
primary spelling. I think that it's easier to read. nounroll_and_jam is fine 
for compatibility if we'd like. I prefer we have a different syntax that we can 
use consistently within the 'clang loop' pragmas. How about 'unroll_and_jam 
disable' or similar?

> If we want to add more transformations, it would be nice to have an explicit 
> naming scheme. E.g. for "register tiling", "stream_unroll" (supported by 
> xlc), "index set splitting", "statement reordering", "strip mine", "overlap 
> tiling", "diamond tiling", "thread-parallelization", "task-parallelization", 
> "loop unswitching", etc.




https://reviews.llvm.org/D47267



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

[PATCH] D47849: [OpenMP][Clang][NVPTX] Enable math functions called in an OpenMP NVPTX target device region to be resolved as device-native function calls

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D47849#1124638, @Hahnfeld wrote:

> IMO this goes into the right direction, we should use the fast implementation 
> in libdevice. If LLVM doesn't lower these calls in the NVPTX backend, I think 
> it's ok to use header wrappers as CUDA already does.
>
> Two questions:
>
> 1. Can you explain where this is important for "correctness"? Yesterday I 
> compiled a code using `sqrt` and it seems to spit out the correct results. 
> Maybe that's relevant for other functions?
> 2. Incidentally I ran into a closely related problem: I can't `#include 
> ` in translation units compiled for offloading, Clang complains about 
> inline assembly for x86 (see below). Does that work for you?
>
>   ``` In file included from /usr/include/math.h:413: 
> /usr/include/bits/mathinline.h:131:43: error: invalid input constraint 'x' in 
> asm __asm ("pmovmskb %1, %0" : "=r" (__m) : "x" (__x)); ^ 
> /usr/include/bits/mathinline.h:143:43: error: invalid input constraint 'x' in 
> asm __asm ("pmovmskb %1, %0" : "=r" (__m) : "x" (__x)); ^ 2 errors generated. 
> ```


Hrmm. I thought that we had fixed that already.

In case it's helpful, in an out-of-tree experimental target I have I ran into a 
similar problem, and to fix that I wrote the following code in the target's 
getTargetDefines function (in lib/Basic/Targets):

  // If used as an OpenMP target on x86, x86 target feature macros are defined. 
math.h
  // and other system headers will include inline asm if these are defined.
  Builder.undefineMacro("__SSE2_MATH__");
  Builder.undefineMacro("__SSE_MATH__");


Repository:
  rC Clang

https://reviews.llvm.org/D47849



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

[PATCH] D6260: Add -mlong-double-64 flag

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added inline comments.



Comment at: lib/Basic/TargetInfo.cpp:277
+LongDoubleWidth = 64;
+LongDoubleAlign = 32;
+LongDoubleFormat = &llvm::APFloat::IEEEdouble;

This seems wrong for targets in general. Maybe this should be, instead of 32 
always, 64 on 64-bit targets?


https://reviews.llvm.org/D6260



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

[PATCH] D53928: Enable builtins necessary for SLEEF [AArch64] vectorized trigonometry libm functions

[Hal Finkel via Phabricator via cfe-commits]

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

LGTM

Please, in the future, make sure you post full-context patches.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53928



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

[PATCH] D54654: Correct CreateAlignmentAssumption to check sign and power-of-2 (Clang Part)

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

> As discussed on IRC, check sign/power of 2 correctly for the alignment 
> assumptions.

"As discussed on IRC" is not appropriate for a commit message. The rationale 
must be documented here.

> I hope that extra is-power-of-two won't confuse the optimizer.

Probably would unless it gets constant folded.


Repository:
  rC Clang

https://reviews.llvm.org/D54654



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

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D53157#1291977, @andrew.w.kaylor wrote:

> Craig Topper also raised some concerns with me (in person, his desk is right 
> by mine) about the potential effect this might have on code size. He tells me 
> that IRBuilder calls are intended to always be inlined and if we grow the 
> implementation of these functions too much it could lead to noticeable bloat. 
> It still seems to me like it might be worthwhile for the simplification it 
> would allow in the front end, but I'm not really a front end guy so I 
> definitely agree that we should get some input from front end people about 
> what they want.


Craig's right about not wanting to bloat the inlinable functions in IRBuilder, 
but this is something that we can measure. In addition, we might be able to 
move the "slow path" (which create the constrained intrinsics) to the .cpp file 
(by manually outlining to a different function).


https://reviews.llvm.org/D53157



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

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

> Just because FENV_ACCESS can be toggled on that granularity doesn't mean we 
> have to represent it that way. We've previously agreed (I think) that if 
> FENV_ACCESS is enabled anywhere in a function we will want to use the 
> constrained intrinsics for all FP operations in the function, not just the 
> ones in the scope where it was specified.

Yes, this is also my understanding. We can't soundly mix the two in the same 
function because we can't prevent the code motion within the function.


https://reviews.llvm.org/D53157



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

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D53157#1301991, @hfinkel wrote:

> In https://reviews.llvm.org/D53157#1291977, @andrew.w.kaylor wrote:
>
> > Craig Topper also raised some concerns with me (in person, his desk is 
> > right by mine) about the potential effect this might have on code size. He 
> > tells me that IRBuilder calls are intended to always be inlined and if we 
> > grow the implementation of these functions too much it could lead to 
> > noticeable bloat. It still seems to me like it might be worthwhile for the 
> > simplification it would allow in the front end, but I'm not really a front 
> > end guy so I definitely agree that we should get some input from front end 
> > people about what they want.
>
>
> Craig's right about not wanting to bloat the inlinable functions in 
> IRBuilder, but this is something that we can measure. In addition, we might 
> be able to move the "slow path" (which create the constrained intrinsics) to 
> the .cpp file (by manually outlining to a different function).


Also, to be clear, adding a mode that automatically adds these when using the 
existing IRBuilder functions seems worth investigating. It seems like that 
would greatly simply the FE code.


https://reviews.llvm.org/D53157



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

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D53157#1301782, @cameron.mcinally wrote:

> Fair enough. Just for conversation's sake, I was envisioning the FE support a 
> little differently...
>
> 1. Add a command line option, say `-ffpe_safe=[true|false]`, that toggles 
> FPEnv-safe code generation for a whole file. A `-ffpe_safe=true` would be 
> equivalent to `FENV_ACCESS=ON` at the beginning of a translation unit and we 
> would capture it in some FE variable. That command line option can be 
> overridden by the FENV_ACCESS pragma.


To bikeshed, I'd prefer we don't have an underscore in the name of the 
command-line flag.

> 
> 
> 2. If the FENV_ACCESS pragma is seen, carry a nop/metadata/something 
> construct to toggle the FE variable during IR generation.
> 3. Then when generating IR, choose between constrained/unconstrained IR 
> depending on the state of the FE variable.
> 
>   #1 may seem superfluous, but there are certain benchmarks (e.g. SPEC CPU) 
> that do not allow for code modifications. So, in order to run those with 
> FPEnv-safe code, we'd need a non-invasive way to toggle FPEnv-safe code 
> generation.
> 
>   I'll also add that my interpretation of FENV_ACCESS in the C Standard 
> [7.6.1] implies that we have to track the FENV_ACCESS pragma. That is, it's 
> not okay to ignore FENV_ACCESS=OFF. When we transfer from FENV_ACCESS=OFF 
> code to FENV_ACCESS=ON code, the rounding mode needs to be reset to the 
> default setting. But, I'm open to hearing other interpretations. My copy of 
> the Standard is old, so please correct me if this changed...
> 
>   ``` When execution passes from a part of the program translated with 
> FENV_ACCESS ‘‘off’’ to a part translated with FENV_ACCESS ‘‘on’’, the state 
> of the floating-point status flags is unspecified and the floating-point 
> control modes have their default settings. ```

The rounding mode does need to be reset to its default setting when passing 
from FENV_ACCESS "on" to FENV_ACCESS "off", but that seems to be the user's 
responsibility. Are you saying that the implementation should reset it on that 
transition?


https://reviews.llvm.org/D53157



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

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D53157#1302159, @cameron.mcinally wrote:

> In https://reviews.llvm.org/D53157#1301992, @hfinkel wrote:
>
> > > Just because FENV_ACCESS can be toggled on that granularity doesn't mean 
> > > we have to represent it that way. We've previously agreed (I think) that 
> > > if FENV_ACCESS is enabled anywhere in a function we will want to use the 
> > > constrained intrinsics for all FP operations in the function, not just 
> > > the ones in the scope where it was specified.
> >
> > Yes, this is also my understanding. We can't soundly mix the two in the 
> > same function because we can't prevent the code motion within the function.
>
>
> Ugh, I don't know. The C Standard's language is so vague.


To be clear, I mean here that *we* can't mix the two soundly in the same 
function (where we generate some operations using the constrained instrinsics 
and some not) because of constraints imposed by our design. The standard may 
indeed allow for more precision. We'll need to be conservatively correct.

> 
> 
> In https://reviews.llvm.org/D53157#1301994, @hfinkel wrote:
> 
>> The rounding mode does need to be reset to its default setting when passing 
>> from FENV_ACCESS "on" to FENV_ACCESS "off", but that seems to be the user's 
>> responsibility. Are you saying that the implementation should reset it on 
>> that transition?
> 
> 
> Yes, that's how I was interpreting the Standard (today). The implementation 
> should reset the control modes. The verbiage is murky at best though.

I'll also point out that whether or not we insert a rounding-mode reset when 
the pragma changes state is orthogonal to whether we stop emitting constrained 
intrinsics at that point.

> We touched on this in https://reviews.llvm.org/D43142 and I do realize that 
> my opinion has flip-flopped since then. I previously believed that reseting 
> the control modes was up to the user, but now I'm not so sure. I suppose that 
> either way, as long as a fesetround(default_mode_constant) is seen with a 
> FENV_ACCESS=OFF, we could use that as a barrier to prevent the problematic 
> code motion.
> 
> Thinking aloud, maybe we should be working on redefining FENV_ACCESS in the C 
> Standard? It's pretty clear that this section could use some work.
> 
> All that said, my understanding of $7.6.1 in the Standard is cloudy at best. 
> If I'm the only one that feels this way, I'll drop my objections...

I don't read the standard that way, but the standard could certainly be more 
clear.


https://reviews.llvm.org/D53157



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

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D53157#1304873, @cameron.mcinally wrote:

> In https://reviews.llvm.org/D53157#1304275, @kpn wrote:
>
> > In https://reviews.llvm.org/D53157#1303398, @cameron.mcinally wrote:
> >
> > > If we all agree upon that, then we simply have to treat the functions 
> > > that modify the FPEnv, e.g. fesetexcept(...), as barriers. That way it 
> > > does not matter if a FENV_ACCESS=OFF function is translated with 
> > > constrained intrinsics or not, since nothing can be scheduled around 
> > > these barriers.
> >
> >
> > I thought we couldn't do barriers. No barriers means no way to prevent code 
> > motion and mixing of constrained with non-constrained FP. That was the 
> > reason for having all FP in a function be constrained if any of it was.
>
>
> I'd like to hear more about this. The fesetexcept(...) function and friends 
> change the FPEnv state, which can change the semantics for the instructions 
> that follow. They definitely have to act as a barrier.


There's no sense in which we can have a code-motion barrier within a function 
that acts on the regular FP instructions. We can have a barrier for the 
constrained intrinsics. This is why we need, in this design, for any function 
that uses FENV_ACCESS=ON for any part of it, to always use the constrained 
instrinsics.


https://reviews.llvm.org/D53157



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

[PATCH] D52117: Generate llvm.loop.parallel_accesses instead of llvm.mem.parallel_loop_access metadata.

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added inline comments.



Comment at: lib/CodeGen/CGLoopInfo.cpp:372
+  if (Active.size() >= 2) {
+LoopInfo &NewFront = reverse(Active).begin()[1];
+NewFront.addAccGroups(Front.getNestedAccGroups());

reverse(Active).begin() looks odd. Can we get the same thing by calling last()?


Repository:
  rC Clang

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

https://reviews.llvm.org/D52117



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

[PATCH] D47849: [OpenMP][Clang][NVPTX] Enable math functions called in an OpenMP NVPTX target device region to be resolved as device-native function calls

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

We need to make progress on this, and I'd like to suggest a path forward...

First, we have a fundamental problem here: Using host headers to declare 
functions for the device execution environment isn't sound. Those host headers 
can do anything, and while some platforms might provide a way to make the host 
headers more friendly (e.g., by defining __NO_MATH_INLINES), these mechanisms 
are neither robust nor portable. Thus, we should not rely on host headers to 
define functions that might be available on the device. However, even when 
compiling for the device, code meant only for host execution must be 
semantically analyzable. This, in general, requires the host headers. So we 
have a situation in which we must both use the host headers during device 
compilation (to keep the semantic analysis of the surrounding host code 
working) and also can't use the host headers to provide definitions for use for 
device code (e.g., because those host headers might provide definitions relying 
on host inline asm, intrinsics, using types not lowerable in device code, could 
provide declarations using linkage-affecting attributes not lowerable for the 
device, etc.).

This is, or is very similar to, the problem that the host/device overloading 
addresses in CUDA. It is also the problem, or very similar to the problem, that 
the new OpenMP 5 `declare variant` directive is intended to address. Johannes 
and I discussed this earlier today, and I suggest that we:

1. Add a math.h wrapper to clang/lib/Headers, which generally just does an 
include_next of math.h, but provides us with the ability to customize this 
behavior. Writing a header for OpenMP on NVIDIA GPUs which is essentially 
identical to the math.h functions in __clang_cuda_device_functions.h would be 
unfortunate, and as CUDA does provide the underlying execution environment for 
OpenMP target offload on NVIDIA GPUs, duplicative even in principle. We don't 
need to alter the default global namespace, however, but can include this file 
from the wrapper math.h.
2. We should allow host/device overloading in OpenMP mode. As an extension, we 
could directly reuse the CUDA host/device overloading capability - this also 
has the advantage of allowing us to directly reuse 
__clang_cuda_device_functions.h (and perhaps do a similar thing to pick up the 
device-side printf, etc. from __clang_cuda_runtime_wrapper.h). In the future, 
we can extend these to provide overloading using OpenMP declare variant, if 
desired, when in OpenMP mode.

Thoughts?


Repository:
  rC Clang

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

https://reviews.llvm.org/D47849



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

[PATCH] D60406: Move the builtin headers to use the new license file header.

[Hal Finkel via Phabricator via cfe-commits]

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

LGTM.  Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60406



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

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In D59712#1472318 , @jdenny wrote:

> In D59712#1469760 , @lebedev.ri 
> wrote:
>
> > In D59712#1469693 , @jdenny wrote:
> >
> > > In D59712#1469392 , @lebedev.ri 
> > > wrote:
> > >
> > > > In D59712#1469358 , @jdenny 
> > > > wrote:
> > > >
> > > > > In D59712#1469301 , 
> > > > > @lebedev.ri wrote:
> > > > >
> > > > > > In D59712#1469295 , 
> > > > > > @craig.topper wrote:
> > > > > >
> > > > > > > Wondering if it would be better to assert for asking for the sign 
> > > > > > > of an unsigned APSInt. I could see a caller just wanting to get 
> > > > > > > the msb for some reason and not knowing that isNegative won’t 
> > > > > > > work.
> > > > > >
> > > > > >
> > > > > > Yes, i, too, would think an assert is much better solution (since i 
> > > > > > literally just tripped over this in this review.)
> > > > >
> > > >
> > > >
> > > > Does this pass `check-all`? `check-all` of stage-2? test-suite?
> > >
> > >
> > > No.  The assert breaks cases in at least ExprConstant.cpp and 
> > > SemaExpr.cpp.
> >
> >
> > Err, i was talking about the current code in the patch :)
>
>
> For me, check-all's success is not affected by the current patch.  I built 
> all subprojects except llgo, which gave me a build problem independently of 
> this patch.
>
> I've never tried running the other tests you mention, for any patch.  I 
> thought people normally left those to the bots.  Should this patch be handled 
> differently?


We have a lot of people actively working off of trunk, and we try very hard to 
keep trunk clean. The bots are a second line of defense, not the primary 
checkers. In any case, this comes down to professional judgement. It is not 
uncommon to ask for a patch author to check self hosting and a test suite run 
before committing - specifically, those patches that might affect correctness, 
or introduce other subtle problems, and for which running the compiler over a 
bunch of C/C++ code might uncover a problem.

Also, is this review now missing some files? I see here only updates to  
APSInt.h (only adding functions), APSIntTest.cpp, and a bunch of tests. Nothing 
that would cause changes to the tests, however (maybe I'm just missing 
something).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59712



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

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In D59712#1472544 , @jdenny wrote:

> In D59712#1472358 , @hfinkel wrote:
>
> > > I've never tried running the other tests you mention, for any patch.  I 
> > > thought people normally left those to the bots.  Should this patch be 
> > > handled differently?
> >
> > We have a lot of people actively working off of trunk, and we try very hard 
> > to keep trunk clean. The bots are a second line of defense, not the primary 
> > checkers. In any case, this comes down to professional judgement. It is not 
> > uncommon to ask for a patch author to check self hosting and a test suite 
> > run before committing - specifically, those patches that might affect 
> > correctness, or introduce other subtle problems, and for which running the 
> > compiler over a bunch of C/C++ code might uncover a problem.
>
>
> Thanks for explaining.  It's my first time receiving these particular 
> requests (probably because of what parts of LLVM I normally edit), so I 
> wasn't sure I understood.


No problem.

> For self-hosting, is it best to build again with CMAKE_C_COMPILER and 
> CMAKE_CXX_COMPILE pointing into the previous build, or is there a better 
> approach?

That's what I do.

> 
> 
>> Also, is this review now missing some files? I see here only updates to  
>> APSInt.h (only adding functions), APSIntTest.cpp, and a bunch of tests. 
>> Nothing that would cause changes to the tests, however (maybe I'm just 
>> missing something).
> 
> All looks fine to me.  The APSInt.h changes are the reason for all the test 
> changes.

Indeed. I forgot that you were changing overrides.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59712



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

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Hal Finkel via Phabricator via cfe-commits]

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

In D59712#1473223 , @jdenny wrote:

> In D59712#1469392 , @lebedev.ri 
> wrote:
>
> > Does this pass `check-all`? `check-all` of stage-2? test-suite?
>
>
> For me, all these tests behave with the current patch.  As before, the only 
> subproject I did not build was llgo.


Great. LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59712



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

[PATCH] D60907: [OpenMP] Add math functions support in OpenMP offloading

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In D60907#1479118 , @gtbercea wrote:

> Ping @hfinkel @tra


The last two comments in D47849  indicated 
exploration of a different approach, and one which still seems superior to this 
one. Can you please comment on why you're now pursuing this approach instead?


Repository:
  rC Clang

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

https://reviews.llvm.org/D60907



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

[PATCH] D60907: [OpenMP] Add math functions support in OpenMP offloading

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In D60907#1479370 , @gtbercea wrote:

> In D60907#1479142 , @hfinkel wrote:
>
> > In D60907#1479118 , @gtbercea 
> > wrote:
> >
> > > Ping @hfinkel @tra
> >
> >
> > The last two comments in D47849  indicated 
> > exploration of a different approach, and one which still seems superior to 
> > this one. Can you please comment on why you're now pursuing this approach 
> > instead?
>
>
> ...
>
> Hal, as far as I can tell, this solution is similar to yours but with a 
> slightly different implementation. If there are particular aspects about this 
> patch you would like to discuss/give feedback on please let me know.


The solution I suggested had the advantages of:

1. Being able to directly reuse the code in `__clang_cuda_device_functions.h`. 
On the other hand, using this solution we need to implement a wrapper function 
for every math function. When `__clang_cuda_device_functions.h` is updated, we 
need to update the OpenMP wrapper as well.
2. Providing access to wrappers for other CUDA intrinsics in a natural way 
(e.g., rnorm3d) [it looks a bit nicer to provide a host version of rnorm3d than 
__nv_rnorm3d in user code].
3. Being similar to the "declare variant" functionality from OpenMP 5, and 
thus, I suspect, closer to the solution we'll eventually be able to apply in a 
standard way to all targets.

What are all of the long-double functions going to do on NVPTX?

> This solution is following Alexey's suggestions. This solution allows the 
> optimization of math calls if they apply (example: pow(x,2) => x*x ) which 
> was one of the issues in the previous solution I implemented.

So we're also missing that optimization for CUDA code when compiling with 
Clang? Isn't this also something that, regardless, should be fixed?

Also, how fragile is this? We inline bottom up but this optimization needs to 
apply before inlining?

Finally, regardless of all of this, do we really need to preinclude this 
header? Can't we do this with a math.h wrapper?


Repository:
  rC Clang

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

https://reviews.llvm.org/D60907



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

[PATCH] D60907: [OpenMP] Add math functions support in OpenMP offloading

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In D60907#1483660 , @jdoerfert wrote:

> In D60907#1483615 , @hfinkel wrote:
>
> > In D60907#1479370 , @gtbercea 
> > wrote:
> >
> > > In D60907#1479142 , @hfinkel 
> > > wrote:
> > >
> > > > In D60907#1479118 , @gtbercea 
> > > > wrote:
> > > >
> > > > > Ping @hfinkel @tra
> > > >
> > > >
> > > > The last two comments in D47849  
> > > > indicated exploration of a different approach, and one which still 
> > > > seems superior to this one. Can you please comment on why you're now 
> > > > pursuing this approach instead?
> > >
> > >
> > > ...
> > >
> > > Hal, as far as I can tell, this solution is similar to yours but with a 
> > > slightly different implementation. If there are particular aspects about 
> > > this patch you would like to discuss/give feedback on please let me know.
> >
> >
> > The solution I suggested had the advantages of:
> >
> > 1. Being able to directly reuse the code in 
> > `__clang_cuda_device_functions.h`. On the other hand, using this solution 
> > we need to implement a wrapper function for every math function. When 
> > `__clang_cuda_device_functions.h` is updated, we need to update the OpenMP 
> > wrapper as well.
>
>
> I'd even go as far as to argue that `__clang_cuda_device_functions.h` should 
> include the internal math.h wrapper to get all math functions. See also the 
> next comment.
>
> > 2. Providing access to wrappers for other CUDA intrinsics in a natural way 
> > (e.g., rnorm3d) [it looks a bit nicer to provide a host version of rnorm3d 
> > than __nv_rnorm3d in user code].
>
> @hfinkel 
>  I don't see why you want to mix CUDA intrinsics with math.h overloads.


What I had in mind was matching non-standard functions in a standard way. For 
example, let's just say that I have a CUDA kernel that uses the rnorm3d 
function, or I otherwise have a function that I'd like to write in OpenMP that 
will make good use of this CUDA function (because it happens to have an 
efficient device implementation). This is a function that CUDA provides, in the 
global namespace, although it's not standard.

Then I can do something like this (depending on how we setup the 
implementation):

  double rnorm3d(double a,  double b, double c) {
return sqrt(a*a + b*b + c*c);
  }
  
  ...
  
  #pragma omp target
  {
double a = ..., b = ..., c = ...;
double r = rnorm3d(a, b, c)
  }

and, if we use the CUDA math headers for CUDA math-function support, than this 
might "just work." To be clear, I can see an argument for having this work 
being a bad idea ;) -- but it has the advantage of providing a way to take 
advantage of system-specific functions while still writing completely-portable 
code.

>   I added a rough outline of how I imagined the internal math.h header to 
> look like as a comment in D47849. Could you elaborate how that differs from 
> what you imagine and how the other intrinsics come in?

That looks like what I had in mind (including `__clang_cuda_device_functions.h` 
to get the device functions.)

> 
> 
>> 3. Being similar to the "declare variant" functionality from OpenMP 5, and 
>> thus, I suspect, closer to the solution we'll eventually be able to apply in 
>> a standard way to all targets.
> 
> I can see this.
> 
>>> This solution is following Alexey's suggestions. This solution allows the 
>>> optimization of math calls if they apply (example: pow(x,2) => x*x ) which 
>>> was one of the issues in the previous solution I implemented.
>> 
>> So we're also missing that optimization for CUDA code when compiling with 
>> Clang? Isn't this also something that, regardless, should be fixed?
> 
> Maybe through a general built-in recognition and lowering into target 
> specific implementations/intrinsics late again?

I suspect that we need to match the intrinsics and perform the optimizations in 
LLVM at that level in order to get the optimizations for CUDA.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60907



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

[PATCH] D61399: [OpenMP][Clang] Support for target math functions

[Hal Finkel via Phabricator via cfe-commits]

hfinkel accepted this revision.
hfinkel added inline comments.
This revision is now accepted and ready to land.



Comment at: lib/Driver/ToolChains/Clang.cpp:1163
+  llvm::sys::path::append(P, "openmp_wrappers");
+  CmdArgs.push_back("-internal-isystem");
+  CmdArgs.push_back(Args.MakeArgString(P));

Please add a driver test covering this.



Comment at: lib/Headers/openmp_wrappers/__clang_openmp_math.h:20
+/// respectively. This is actually what the Clang-CUDA code path does, using
+/// __device__ instead of variants to avoid redeclarations and get the desired
+/// overload resolution.

And this is why I wanted to just make `__device__` work in OpenMP mode. But, as 
a temporary solution until we get `declare variant` working, I'm okay with this.

Modulo the naming nit and the missing driver test, LGTM.



Comment at: lib/Headers/openmp_wrappers/__clang_openmp_math.h:46
+/// Magic macro for stopping the math.h/cmath host header from being included.
+#define __NO_HOST_MATH__
+

I'd prefer that, as this is clang specific, we make this clear by naming this 
`__CLANG_NO_HOST_MATH__`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61399



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

[PATCH] D61399: [OpenMP][Clang] Support for target math functions

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In D61399#1488262 , @ABataev wrote:

> I don't like this implementation. Seems to me, it breaks one of the OpenMP 
> standard requirements: the program can be compiled without openmp support. I 
> assume, that with this includes the program won't be able to be compiled 
> without OpenMP support anymore because it may use some device-specific math 
> functions explicitly.
>  Instead, I would like to see some additional, device-scpecific math header 
> file, that must be included explicitly to support some device-specific math 
> functions. And we need to provide default implementations for those extra 
> math functions for all the platforms we're going to support, including 
> default host implementations.


Can you provide an example of a conforming program that can't be compiled 
without OpenMP support? Regardless of the use of any device-specific functions 
(which isn't covered by the standard, of course, but might be needed in 
practice), the code still needs to be compilable by the host in order to 
generate the host-fallback version. This doesn't change that. Thus, any program 
that uses anything from this math.h, etc. needs to compile for the host, and 
thus, likely compiles without OpenMP support. Maybe I'm missing your point, 
however.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61399



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

[PATCH] D61399: [OpenMP][Clang] Support for target math functions

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In D61399#1488309 , @ABataev wrote:

> In D61399#1488299 , @hfinkel wrote:
>
> > In D61399#1488262 , @ABataev wrote:
> >
> > > I don't like this implementation. Seems to me, it breaks one of the 
> > > OpenMP standard requirements: the program can be compiled without openmp 
> > > support. I assume, that with this includes the program won't be able to 
> > > be compiled without OpenMP support anymore because it may use some 
> > > device-specific math functions explicitly.
> > >  Instead, I would like to see some additional, device-scpecific math 
> > > header file, that must be included explicitly to support some 
> > > device-specific math functions. And we need to provide default 
> > > implementations for those extra math functions for all the platforms 
> > > we're going to support, including default host implementations.
> >
> >
> > Can you provide an example of a conforming program that can't be compiled 
> > without OpenMP support? Regardless of the use of any device-specific 
> > functions (which isn't covered by the standard, of course, but might be 
> > needed in practice), the code still needs to be compilable by the host in 
> > order to generate the host-fallback version. This doesn't change that. 
> > Thus, any program that uses anything from this math.h, etc. needs to 
> > compile for the host, and thus, likely compiles without OpenMP support. 
> > Maybe I'm missing your point, however.
>
>
> Assume we have something like this:
>
>   #pragma omp target if(cond)
>   a = __nv_();
>
>
> Instead of `__nv_xxx` you can try to use any Cuda-specific function, which is 
> not the part of the standard `math.h`/`cmath` files. Will it be compilable 
> even with OpenMP?


I don't think that this changes that one way or the other. Your example won't 
work, AFAIK, unless you do something like:

  #pragma omp target if(cond)
  #ifdef __NVPTX__
  a = __nv_();
  #else
  a = something_on_the_host;
  #endif

and anything from these headers that doesn't also have a host version will 
suffer the same fate: if it won't also compile for the host (one way or 
another), then it won't work.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61399



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

[PATCH] D61399: [OpenMP][Clang] Support for target math functions

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In D61399#1488366 , @ABataev wrote:

> In D61399#1488329 , @hfinkel wrote:
>
> > In D61399#1488309 , @ABataev wrote:
> >
> > > In D61399#1488299 , @hfinkel 
> > > wrote:
> > >
> > > > In D61399#1488262 , @ABataev 
> > > > wrote:
> > > >
> > > > > I don't like this implementation. Seems to me, it breaks one of the 
> > > > > OpenMP standard requirements: the program can be compiled without 
> > > > > openmp support. I assume, that with this includes the program won't 
> > > > > be able to be compiled without OpenMP support anymore because it may 
> > > > > use some device-specific math functions explicitly.
> > > > >  Instead, I would like to see some additional, device-scpecific math 
> > > > > header file, that must be included explicitly to support some 
> > > > > device-specific math functions. And we need to provide default 
> > > > > implementations for those extra math functions for all the platforms 
> > > > > we're going to support, including default host implementations.
> > > >
> > > >
> > > > Can you provide an example of a conforming program that can't be 
> > > > compiled without OpenMP support? Regardless of the use of any 
> > > > device-specific functions (which isn't covered by the standard, of 
> > > > course, but might be needed in practice), the code still needs to be 
> > > > compilable by the host in order to generate the host-fallback version. 
> > > > This doesn't change that. Thus, any program that uses anything from 
> > > > this math.h, etc. needs to compile for the host, and thus, likely 
> > > > compiles without OpenMP support. Maybe I'm missing your point, however.
> > >
> > >
> > > Assume we have something like this:
> > >
> > >   #pragma omp target if(cond)
> > >   a = __nv_();
> > >
> > >
> > > Instead of `__nv_xxx` you can try to use any Cuda-specific function, 
> > > which is not the part of the standard `math.h`/`cmath` files. Will it be 
> > > compilable even with OpenMP?
> >
> >
> > I don't think that this changes that one way or the other. Your example 
> > won't work, AFAIK, unless you do something like:
> >
> >   #pragma omp target if(cond)
> >   #ifdef __NVPTX__
> >   a = __nv_();
> >   #else
> >   a = something_on_the_host;
> >   #endif
> >   
> >
> > and anything from these headers that doesn't also have a host version will 
> > suffer the same fate: if it won't also compile for the host (one way or 
> > another), then it won't work.
>
>
> The problem with this header file is that it allows to use those 
> Cuda-specific functions unconditionally in some cases:
>
>   #pragma omp target
>   a = __nv_();
>
>
> It won't require any target-specific guards to compile this code (if we 
> compile it only for Cuda-specific devices) and we're loosing the consistency 
> here: in some cases target regions will require special device guards, in 
> others, with the same function calls, it is not. And the worst thing, is that 
> we implicitly allow to introduce this kind of incostistency into users code. 
> That's why I would prefer to see a special kind of the include file, NVPTX 
> specific, that must be included explicitly, so the user explictly commanded 
> to use some target-specific math functions, if he really wants it. Plus, 
> maybe, in this files we need force check of the platform and warn users that 
> the functions from this header file must be used using device-specific 
> checks. Or provide some kind of the default implementations for all the 
> platforms, that do not support those math function natively.


I believe that I understand your point, but two things:

1. I think that you're mistaken on the underlying premise. That code will not 
meaningfully compile without ifdefs, even if only CUDA-specific devices are the 
only ones selected. We *always* compile code for the host as well, not for 
offloading proper, but for the fallback (for execution when the offloading 
fails). If I emulate this situation by writing this:



  #ifdef __NVPTX__
  int __nv_floor();
  #endif
  
  int main() {
  #pragma omp target
  __nv_floor();
  }

and try to compile using Clang with -fopenmp -fopenmp-targets=nvptx64, the 
compilation fails:

  int1.cpp:8:1: error: use of undeclared identifier '__nv_floor'

and this is because, when we invoke the compilation for the host, there is no 
declaration for that function. This is true even though nvptx64 is the only 
target for which the code is being compiled (because we always also compile the 
host fallback).

2. I believe that the future state -- what we get by following this patch, and 
then when declare variant is available using that -- gives us all what we want. 
When we have declare variant, then all of the definitions in these headers will 
be declared as variants only available

[PATCH] D61458: [hip] Relax CUDA call restriction within `decltype` context.

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In D61458#1488970 , @jlebar wrote:

> Here's one for you:
>
>   __host__ float bar();
>   __device__ int bar();
>   __host__ __device__ auto foo() -> decltype(bar()) {}
>
>
> What is the return type of `foo`?  :)
>
> I don't believe the right answer is, "float when compiling for host, int when 
> compiling for device."


So, actually, I wonder if that's not the right answer. We generally allow 
different overloads to have different return types. What if, for example, the 
return type on the host is __float128 and on the device it's `MyLongFloatTy`?

> I'd be happy if we said this was an error, so long as it's well-defined what 
> exactly we're disallowing.  But I bet @rsmith can come up with substantially 
> more evil testcases than this.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61458



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

[PATCH] D61458: [hip] Relax CUDA call restriction within `decltype` context.

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

> Only if they also differ in some other way. C++ does not (generally) have 
> return-type-based overloading. The two functions described would even mangle 
> the same way if CUDA didn't include host/device in the mangling.

Certainly. I didn't mean to imply otherwise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61458



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

[PATCH] D61399: [OpenMP][Clang] Support for target math functions

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

> Still, I think we need to prvide the default implementation of those 
> non-standard functions (they can be very simple, maybe reporting error is 
> going to be enough), which can be overriden by user.

I appreciate your motivation, and I agree with you to some extent. I don't 
object to having generic versions of useful math functions, but I don't think 
they should be required. It's not reasonable to make someone add generic 
versions of every function which happens to appear in a system/target-specific 
math.h header. NVPTX won't be the only target that has target-optimized 
functions that get pulled in, even from our own headers, but system headers 
also have differences anyway depending on what preprocessor macros are defined. 
In the end, people can write portable code if they stick to what's in the 
standard, and we should make it reasonably easy for them to step outside of the 
standard to do what they need to do when the standard subset of available 
functionality doesn't meet their needs for whatever reason. This is what we do 
for C/C++, where we provide intrinsics and other system functions for those who 
can't write their code only using the facilities that C/C++ provide.

In any case, I think that we can figure out how to add generic versions of 
non-standard math functions in a separate thread. I think that we should move 
forward with this and then make progress on generic versions separately. It's 
also possible that we want to fold this discussion into the discussion on an 
LLVM math library (we've talked about this for some time in the context of 
vector math libraries, and I'd not thought about accelerators in this context, 
but maybe this is all related).


Repository:
  rC Clang

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

https://reviews.llvm.org/D61399



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

[PATCH] D61399: [OpenMP][Clang] Support for target math functions

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

> It is up to you. I don't have strong objections if you think this will work 
> as required. Just the tests must be fixed, especially codegen tests.

Thanks, Alexey. I think this will work as required, and then we'll be able to 
update it when we get declare variant. Agreed on the tests (on all points).


Repository:
  rC Clang

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

https://reviews.llvm.org/D61399



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

[PATCH] D61399: [OpenMP][Clang] Support for target math functions

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added inline comments.



Comment at: lib/Headers/CMakeLists.txt:36
   bmiintrin.h
+  openmp_wrappers/math.h
+  openmp_wrappers/cmath

JDevlieghere wrote:
> This doesn't do what you think it would do. The files are copied into the 
> root of the resource directory, which causes stage 2 build failures on 
> GreenDragon. 
Can you provide a link to the failure log? Is the problem that the files are 
not copied into their subdirectory?


Repository:
  rC Clang

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

https://reviews.llvm.org/D61399



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

[PATCH] D58128: [PowerPC] Stop defining _ARCH_PWR6X on POWER7 and up

[Hal Finkel via Phabricator via cfe-commits]

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

LGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D58128



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

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D37042#852733, @efriedma wrote:

> It would be nice to warn on any nullptr arithmetic, yes.  (We probably want 
> the wording of the warning to be a bit different if we're activating this 
> workaround.)


+1 (I'll likely want the ability to turn off the warning for one without 
turning off the warning for the other)


https://reviews.llvm.org/D37042



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

[PATCH] D37436: Initial implementation of C attributes (WG14 N2137)

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D37436#867965, @aaron.ballman wrote:

> In https://reviews.llvm.org/D37436#867287, @rsmith wrote:
>
> > If this is just supposed to be an experiment to get feedback on the 
> > feature,  then I don't think we should be treating it as a different 
> > attribute syntax at all. Rather, I think we
> >  just want to permit C++11 attributes to be parsed in other language modes. 
> > If/when this becomes part of some future C working draft, I think that's 
> > the time to have a
> >  separate attribute syntax with a distinct set of valid unqualified 
> > attribute names.
>
>
> I do not think that's the correct approach. These are not C++ attributes (for 
> instance, no `using` insanity is allowed, `::` is a new lexing token in C, 
> etc). Also, I don't think it's a good idea to enable all C++11-style 
> attributes in C mode without giving each attribute some appropriate thought 
> (what does `abi_tag` *do* in C mode? What happens with _Noreturn vs 
> [[noreturn]] etc). Also, I'm not comfortable adding a bunch of 
> vendor-specific `gnu::` attributes that GCC does not implement in C yet.


On this last point, I disagree. Implementation experience is about all of the 
messy things that occur in production. In production, if we have this syntax, 
then we'll end up enabling it for a bunch of vendor-specific attributes. Do you 
think that we wouldn't? N2137 specifically talks about this as a use case. If 
so, this will include `gnu::` attributes that we have in Clang (even if GCC 
does not implement them). From my perspective, lack of consistency here between 
Clang's C and C++ modes is much more problematic than a lack of consistency 
between what Clang and GCC implement.


https://reviews.llvm.org/D37436



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

[PATCH] D37436: Initial implementation of C attributes (WG14 N2137)

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D37436#868333, @aaron.ballman wrote:

> In https://reviews.llvm.org/D37436#868295, @hfinkel wrote:
>
> > In https://reviews.llvm.org/D37436#867965, @aaron.ballman wrote:
> >
> > > In https://reviews.llvm.org/D37436#867287, @rsmith wrote:
> > >
> > > > If this is just supposed to be an experiment to get feedback on the 
> > > > feature,  then I don't think we should be treating it as a different 
> > > > attribute syntax at all. Rather, I think we
> > > >  just want to permit C++11 attributes to be parsed in other language 
> > > > modes. If/when this becomes part of some future C working draft, I 
> > > > think that's the time to have a
> > > >  separate attribute syntax with a distinct set of valid unqualified 
> > > > attribute names.
> > >
> > >
> > > I do not think that's the correct approach. These are not C++ attributes 
> > > (for instance, no `using` insanity is allowed, `::` is a new lexing token 
> > > in C, etc). Also, I don't think it's a good idea to enable all 
> > > C++11-style attributes in C mode without giving each attribute some 
> > > appropriate thought (what does `abi_tag` *do* in C mode? What happens 
> > > with _Noreturn vs [[noreturn]] etc). Also, I'm not comfortable adding a 
> > > bunch of vendor-specific `gnu::` attributes that GCC does not implement 
> > > in C yet.
> >
> >
> > On this last point, I disagree. Implementation experience is about all of 
> > the messy things that occur in production. In production, if we have this 
> > syntax, then we'll end up enabling it for a bunch of vendor-specific 
> > attributes. Do you think that we wouldn't?
>
>
> I'm sure we would. Also, FWIW, I was planning to traverse the attributes we 
> implement to find which clang-specific C++ attributes would make sense to 
> also enable as a follow-up patch once the syntax is in.
>
> > N2137 specifically talks about this as a use case. If so, this will include 
> > `gnu::` attributes that we have in Clang (even if GCC does not implement 
> > them).
>
> Eventually, yes, but it seems like a problem to implement something under 
> that vendor namespace when the vendor themselves do not. I think it would be 
> really unfortunate were GCC to add a C++ attribute named [[clang::frobble]] 
> that Clang does not implement, and I don't see this case as being all that 
> different. My belief is that GCC will eventually elect to make most of these 
> attributes available in C mode and that's an appropriate time for us to do 
> the same for their vendor namespace.
>
> > From my perspective, lack of consistency here between Clang's C and C++ 
> > modes is much more problematic than a lack of consistency between what 
> > Clang and GCC implement.
>
> From my perspective, they're both problems in their own right. To me (and 
> maybe I'm weird with this line of reasoning), the only reasonable time to 
> implement an attribute under another vendor's attribute namespace is when you 
> are promising your users that you will attempt to match the owning vendor's 
> semantics for that attribute. A case could be made here that the owning 
> vendor *has* implemented that attribute (since they have in C++), but I'm not 
> too comfortable *assuming* that the GCC folks are okay with this since they 
> don't implement the feature syntax in C yet.
>
> That said, I'm happy to ask Jason at the meetings in Albuquerque to explore 
> the idea -- but I don't think it should hold up this patch, especially since 
> we have our own vendor attributes we can use for gaining experience.


I certainly understand your perspective, but this is an orthogonal concern. If 
this is something that Clang does, then it should do it consistently. If you'd 
like us not to support `gnu::` attributes that GCC itself does not support, and 
that's something that we currently do in C++, then please submit a patch to fix 
that for all language modes. It should not differ between language modes.

Is the problem here that we're treating `gnu::`, not as a vendor prefix, but as 
generic escape hatch to get to anything generally provided via GCC-attribute 
syntax (which many compilers, including ours, have extended with attributes 
that GCC does not itself support)?


https://reviews.llvm.org/D37436



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

[PATCH] D37436: Initial implementation of C attributes (WG14 N2137)

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

Also, please post a full-context patch.


https://reviews.llvm.org/D37436



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

[PATCH] D37436: Initial implementation of C attributes (WG14 N2137)

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D37436#869445, @aaron.ballman wrote:

> In https://reviews.llvm.org/D37436#869350, @hfinkel wrote:
>
> > In https://reviews.llvm.org/D37436#868333, @aaron.ballman wrote:
> >
> > > In https://reviews.llvm.org/D37436#868295, @hfinkel wrote:
> > >
> > > > In https://reviews.llvm.org/D37436#867965, @aaron.ballman wrote:
> > > >
> > > > > In https://reviews.llvm.org/D37436#867287, @rsmith wrote:
> > > > >
> > > > > > If this is just supposed to be an experiment to get feedback on the 
> > > > > > feature,  then I don't think we should be treating it as a 
> > > > > > different attribute syntax at all. Rather, I think we
> > > > > >  just want to permit C++11 attributes to be parsed in other 
> > > > > > language modes. If/when this becomes part of some future C working 
> > > > > > draft, I think that's the time to have a
> > > > > >  separate attribute syntax with a distinct set of valid unqualified 
> > > > > > attribute names.
> > > > >
> > > > >
> > > > > I do not think that's the correct approach. These are not C++ 
> > > > > attributes (for instance, no `using` insanity is allowed, `::` is a 
> > > > > new lexing token in C, etc). Also, I don't think it's a good idea to 
> > > > > enable all C++11-style attributes in C mode without giving each 
> > > > > attribute some appropriate thought (what does `abi_tag` *do* in C 
> > > > > mode? What happens with _Noreturn vs [[noreturn]] etc). Also, I'm not 
> > > > > comfortable adding a bunch of vendor-specific `gnu::` attributes that 
> > > > > GCC does not implement in C yet.
> > > >
> > > >
> > > > On this last point, I disagree. Implementation experience is about all 
> > > > of the messy things that occur in production. In production, if we have 
> > > > this syntax, then we'll end up enabling it for a bunch of 
> > > > vendor-specific attributes. Do you think that we wouldn't?
> > >
> > >
> > > I'm sure we would. Also, FWIW, I was planning to traverse the attributes 
> > > we implement to find which clang-specific C++ attributes would make sense 
> > > to also enable as a follow-up patch once the syntax is in.
> > >
> > > > N2137 specifically talks about this as a use case. If so, this will 
> > > > include `gnu::` attributes that we have in Clang (even if GCC does not 
> > > > implement them).
> > >
> > > Eventually, yes, but it seems like a problem to implement something under 
> > > that vendor namespace when the vendor themselves do not. I think it would 
> > > be really unfortunate were GCC to add a C++ attribute named 
> > > [[clang::frobble]] that Clang does not implement, and I don't see this 
> > > case as being all that different. My belief is that GCC will eventually 
> > > elect to make most of these attributes available in C mode and that's an 
> > > appropriate time for us to do the same for their vendor namespace.
> > >
> > > > From my perspective, lack of consistency here between Clang's C and C++ 
> > > > modes is much more problematic than a lack of consistency between what 
> > > > Clang and GCC implement.
> > >
> > > From my perspective, they're both problems in their own right. To me (and 
> > > maybe I'm weird with this line of reasoning), the only reasonable time to 
> > > implement an attribute under another vendor's attribute namespace is when 
> > > you are promising your users that you will attempt to match the owning 
> > > vendor's semantics for that attribute. A case could be made here that the 
> > > owning vendor *has* implemented that attribute (since they have in C++), 
> > > but I'm not too comfortable *assuming* that the GCC folks are okay with 
> > > this since they don't implement the feature syntax in C yet.
> > >
> > > That said, I'm happy to ask Jason at the meetings in Albuquerque to 
> > > explore the idea -- but I don't think it should hold up this patch, 
> > > especially since we have our own vendor attributes we can use for gaining 
> > > experience.
> >
> >
> > I certainly understand your perspective, but this is an orthogonal concern. 
> > If this is something that Clang does, then it should do it consistently. If 
> > you'd like us not to support `gnu::` attributes that GCC itself does not 
> > support, and that's something that we currently do in C++, then please 
> > submit a patch to fix that for all language modes. It should not differ 
> > between language modes.
> >
> > Is the problem here that we're treating `gnu::`, not as a vendor prefix, 
> > but as generic escape hatch to get to anything generally provided via 
> > GCC-attribute syntax (which many compilers, including ours, have extended 
> > with attributes that GCC does not itself support)?
>
>
> I definitely agree that we want to be self-consistent, so thank you for 
> helping me understand where you're coming from.
>
> I've been very consistent in rejecting patches that add C++ attributes to the 
> gnu namespace unless GCC also implements them. This most often comes up as a 
> misunderstan