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

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

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

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

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

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 mi

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

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. http

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

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 --c

[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

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.

[PATCH] D38113: OpenCL: Assume functions are convergent

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

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

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:

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

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, c

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

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 -fins

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

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 m

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

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 ad

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

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 h

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

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.

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

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

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

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

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

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

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/Cod

[PATCH] D38596: Implement attribute target multiversioning

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 th

[PATCH] D38596: Implement attribute target multiversioning

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)?

[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

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().FineGrainedBitfieldAcc

[PATCH] D24933: Enable configuration files in clang

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

[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

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; --

[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

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) ///< E

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

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/Cod

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

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-b

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

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://rev

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

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 th

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

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 "mangli

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

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 everyth

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

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://revie

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

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 > > backen

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

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 t

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

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. > >

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

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. http

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

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

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

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

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

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 ma

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

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 (mos

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

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-

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

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

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

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 "

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

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 ac

[PATCH] D33904: Add a __has_attribute_enhancement macro to clang

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

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

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

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

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-b

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

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 Ope

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

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.

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

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; > Con

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

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->addFnA

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

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 str

[PATCH] D24933: Enable configuration files in clang

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

[PATCH] D40594: [InstCombine] miscompile of __builtin_fmod

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 ___ cf

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

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

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

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/TextDiagnos

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

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/DiagnosticDriverKi

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

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, th

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

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:!.*]]

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

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

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

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:!.*]]

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

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,

[PATCH] D24933: Enable configuration files in clang

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: " + << SystemC

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

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-c

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

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 f

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

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.

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

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 "unro

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

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

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

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,

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

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/D5392

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

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 woul

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

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 alwa

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

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 operation

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

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

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

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 g

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

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 ag

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

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

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

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

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

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

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

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 ___

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

hfinkel added a comment. In D59712#1472318 , @jdenny wrote: > In D59712#1469760 , @lebedev.ri > wrote: > > > In D59712#1469693 , @jdenny wrote: > > > > > In D59712#1469392 <

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

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 t

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

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-al

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

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

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

hfinkel added a comment. In D60907#1479370 , @gtbercea wrote: > In D60907#1479142 , @hfinkel wrote: > > > In D60907#1479118 , @gtbercea > > wrote: > > > > > Ping @hfinkel @t

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

hfinkel added a comment. In D60907#1483660 , @jdoerfert wrote: > In D60907#1483615 , @hfinkel wrote: > > > In D60907#1479370 , @gtbercea > > wrote: > > > > > In D60907#14791

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

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.pus

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

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

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

hfinkel added a comment. In D61399#1488309 , @ABataev wrote: > In D61399#1488299 , @hfinkel wrote: > > > In D61399#1488262 , @ABataev wrote: > > > > > I don't like this imple

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

hfinkel added a comment. In D61399#1488366 , @ABataev wrote: > In D61399#1488329 , @hfinkel wrote: > > > In D61399#1488309 , @ABataev wrote: > > > > > In D61399#1488299

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

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

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

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. R

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

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

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

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. Agre

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

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 dire

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

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

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

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 tur

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

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

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

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: > > > >

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

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)

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: > > >

  1   2   3   4   >