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

[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]] =

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

[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

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

hfinkel added inline comments. Comment at: lib/Basic/TargetInfo.cpp:277 +LongDoubleWidth = 64; +LongDoubleAlign = 32; +LongDoubleFormat = ::APFloat::IEEEdouble; This seems wrong for targets in general. Maybe this should be, instead of 32 always, 64

[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

[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

[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] 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] 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

[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

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

[PATCH] D49114: Add a clang-tidy check for "magic numbers"

hfinkel added a comment. > This version detects and report integers only. If there is interest of > merging the tool I can add the functionality for floats as well. FWIW: I think that the FP check would be interesting. > Also I have seen coding guidelines suggesting "100" is grandfathered due

[PATCH] D48721: Patch to fix pragma metadata for do-while loops

hfinkel added a comment. In https://reviews.llvm.org/D48721#1150677, @bjope wrote: > In https://reviews.llvm.org/D48721#1150361, @hfinkel wrote: > > > In https://reviews.llvm.org/D48721#1150333, @bjope wrote: > > > > > Is the fault that the metadata only should be put on the back edge, not > >

[PATCH] D48721: Patch to fix pragma metadata for do-while loops

hfinkel added a comment. In https://reviews.llvm.org/D48721#1152023, @deepak2427 wrote: > I encountered the issue while working with the unroller and found that it was > not following the pragma info, and traced it back to the issue with metadata. > As far as I understood, for for-loops and

[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

[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

[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

[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

[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

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

[PATCH] D55710: add pragmas to control Software Pipelining optimisation

hfinkel added inline comments. Comment at: include/clang/Basic/Attr.td:2871-2872 /// distribute: attempt to distribute loop if State == Enable + /// pipeline: disable pipelining loop if State == Disable + /// pipeline_ii_count: try to pipeline loop for only 'Value' value

[PATCH] D55928: [OpenMP] Add flag for preventing the extension to 64 bits for the collapse loop counter

hfinkel added inline comments. Comment at: docs/OpenMPSupport.rst:120 +compile your program with the `-fopenmp-optimistic-collapse`. + + Can you please clarify here what happens when the loop induction variables are already 64 bits. If any of them are already

[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 = reverse(Active).begin()[1]; +NewFront.addAccGroups(Front.getNestedAccGroups()); reverse(Active).begin() looks odd. Can we get the same

[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] 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

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

hfinkel accepted this revision. hfinkel added a comment. Minor typo noted below, but otherwise, LGTM (to avoid any misunderstanding: this should be committed after the LLVM change lands). Comment at: lib/CodeGen/CGLoopInfo.cpp:341 +for (const LoopInfo : Active) { +

[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] 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] 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] D61634: [clang/llvm] Allow efficient implementation of libc's memory functions in C/C++

hfinkel added a subscriber: chandlerc. hfinkel added a comment. In D61634#1502201 , @tejohnson wrote: > In D61634#1502138 , @hfinkel wrote: > > > In D61634#1502043 ,

[PATCH] D63329: Allow static linking of libc++ on Linux, just like -static-libstdc++

hfinkel added inline comments. Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:570 + // FIXME: libc++ dynamically links against libpthread, so for static + // linking, we need to supply that dependency. Why does this say FIXME?

[PATCH] D61634: [clang/llvm] Allow efficient implementation of libc's memory functions in C/C++

hfinkel added a comment. In D61634#1502043 , @efriedma wrote: > > I have a related patch that turns -fno-builtin* options into module flags > > Do you have any opinion on representing -fno-builtin using a module flag vs. > a function attribute in IR? It

[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

[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

[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

[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

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

[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"); +

[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

[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

[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

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

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

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

[PATCH] D64067: [X86][PPC] Support -mlong-double-64

hfinkel added a comment. Can you please add a test ensuring that -malign-double and -mlong-double-64 interact properly? I think that, in the current patch, they do (as -malign-double is processed first), but I'd prefer that we cover that case explicitly. Repository: rC Clang CHANGES SINCE

[PATCH] D64067: [X86][PPC] Support -mlong-double-64

hfinkel added a comment. In D64067#156 , @rnk wrote: > In D64067#1568533 , @andrew.w.kaylor > wrote: > > > In this review (https://reviews.llvm.org/D6260) @rsmith mentions that this > > should also have an

[PATCH] D64128: [CodeGen] Generate llvm.ptrmask instead of inttoptr(and(ptrtoint, C)) if possible.

hfinkel added a comment. In D64128#1569590 , @efriedma wrote: > > If they're all syntactically together like this, maybe that's safe? > > Having them together syntactically doesn't really help, I think; it might be > guarded by some code that does the

[PATCH] D64128: [CodeGen] Generate llvm.ptrmask instead of inttoptr(and(ptrtoint, C)) if possible.

hfinkel added a comment. In D64128#1569817 , @rjmccall wrote: > The pointer/integer conversion is "implementation-defined", but it's not > totally unconstrained. C notes that "The mapping functions for converting a > pointer to an integer or an integer

[PATCH] D64128: [CodeGen] Generate llvm.ptrmask instead of inttoptr(and(ptrtoint, C)) if possible.

hfinkel added a comment. In D64128#1568857 , @efriedma wrote: > I don't think this transform is valid, for the same reasons we don't do it in > IR optimizations. I believe that in the problematic cases we previously discussed (e.g., from

[PATCH] D64128: [CodeGen] Generate llvm.ptrmask instead of inttoptr(and(ptrtoint, C)) if possible.

hfinkel added a comment. In D64128#1572504 , @rjmccall wrote: > I would be opposed to any addition of a `-f` flag for this absent any > evidence that it's valuable for some actual C code; this patch appears to be > purely speculative. I certainly don't

[PATCH] D64283: [PowerPC] Support -mabi=ieeelongdouble and -mabi=ibmlongdouble

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/D64283/new/ https://reviews.llvm.org/D64283 ___ cfe-commits

[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] D65410: [PassManager] First Pass implementation at -O1 pass pipeline

hfinkel added a comment. Thanks for starting on this. Can you go ahead and replace the sroa calls with mem2reg calls for `O1` and then see what that does to the performance? That strikes me as a major change, but certainly one that potentially makes sense, so I'd rather we go ahead and test it

[PATCH] D66490: [NewPM] Enable the New Pass Manager by Default in Clang

hfinkel added a comment. In D66490#1638162 , @rupprecht wrote: > We already know that we don't want this enabled for tsan builds due to > https://bugs.llvm.org/show_bug.cgi?id=42877, but I don't even know if anyone > else will hit it (it's only when

[PATCH] D63607: [clang][driver] Add basic --driver-mode=fortran support for flang

hfinkel added inline comments. Comment at: clang/lib/Driver/ToolChains/Flang.cpp:73 + } else { +assert(Output.isNothing() && "Input output."); + } Should this say "Invalid output"? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63607/new/

[PATCH] D63607: [clang][driver] Add basic --driver-mode=flang support for fortran

hfinkel added a comment. > One thought that occurs to me when reviewing this. I think we will assume > that F18 /flang when it lands in the LLVM > project will be an optional thing to build and will be an opt-in project at > the start that is off by default. What

[PATCH] D65880: [Driver] Move LIBRARY_PATH before user inputs

hfinkel accepted this revision. hfinkel added a comment. This revision is now accepted and ready to land. LGTM. Local flags should certainly override LIBRARY_PATH. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65880/new/ https://reviews.llvm.org/D65880

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

hfinkel added inline comments. Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93 ; -#pragma omp target parallel for private(ps) is_device_ptr(ps) // expected-error{{private variable cannot be in a is_device_ptr clause in '#pragma omp target

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

hfinkel added inline comments. Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93 ; -#pragma omp target parallel for private(ps) is_device_ptr(ps) // expected-error{{private variable cannot be in a is_device_ptr clause in '#pragma omp target

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

hfinkel added inline comments. Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93 ; -#pragma omp target parallel for private(ps) is_device_ptr(ps) // expected-error{{private variable cannot be in a is_device_ptr clause in '#pragma omp target

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

hfinkel added a comment. In D65835#1623772 , @ABataev wrote: > In D65835#1623756 , @kkwli0 wrote: > > > In D65835#1622042 , @ABataev wrote: > > > > > > I want to be sure

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

hfinkel added a comment. In D65835#1623903 , @ABataev wrote: > In D65835#1623883 , @hfinkel wrote: > > > In D65835#1623814 , @hfinkel wrote: > > > > > In D65835#1623772

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

hfinkel added a comment. In D65835#1623814 , @hfinkel wrote: > In D65835#1623772 , @ABataev wrote: > > > In D65835#1623756 , @kkwli0 wrote: > > > > > In D65835#1622042

[PATCH] D64067: [X86][PPC] Support -mlong-double-64

hfinkel added a comment. In D64067#1592201 , @troyj wrote: > Hi, we just inherited this commit at Cray when we did our latest upstream > merge and there are a few problems with it that I'd like to point out. Sorry > that I was not part of the initial

[PATCH] D64386: CodeGen: Use memset in initializers for non-zeros

hfinkel added inline comments. Comment at: clang/test/CodeGen/init-memset.c:29 char a[] = "aa"; - // CHECK: call void @llvm.memcpy.{{.*}} + // CHECK: call void @llvm.memset.{{.*}} use(a); I'd be

[PATCH] D64850: Remove use of malloc in PPC mm_malloc.

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/D64850/new/ https://reviews.llvm.org/D64850 ___ cfe-commits

[PATCH] D64128: [CodeGen] Generate llvm.ptrmask instead of inttoptr(and(ptrtoint, C)) if possible.

hfinkel added a comment. In D64128#1570609 , @rjmccall wrote: > In D64128#1569836 , @hfinkel wrote: > > > In D64128#1569817 , @rjmccall > > wrote: > > > > > The

[PATCH] D59919: [Attributor] Deduce "returned" argument attribute

hfinkel accepted this revision. hfinkel added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/test/CodeGenOpenCL/as_type.cl:3 +// Once the attributor is on by default remove the following run line and change the prefixes below. +// RUN:

[PATCH] D64283: [PowerPC] Support -mabi=ieeelongdouble and -mabi=ibmlongdouble

hfinkel added a comment. Ah, fun with overloaded, legacy command-line options... Comment at: lib/Driver/ToolChains/Clang.cpp:1825 + // that don't use the altivec abi. + if (!SeenOther) +ABIName = A->getValue(); This seems like an

[PATCH] D64128: [CodeGen] Generate llvm.ptrmask instead of inttoptr(and(ptrtoint, C)) if possible.

hfinkel added a comment. > Do we have any policies against using clang-only builtins in the codebase? No, but obviously it will need to be protected with appropriate ifdefs and integrated in some maintainable fashion. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D69897: Add #pragma clang loop aligned

hfinkel added inline comments. Comment at: clang/docs/LanguageExtensions.rst:3135 + +This predicates all the array references inside the loop to be aligned. The aligned access to them can increase fetch time and increase the performance. + lebedev.ri wrote: >

[PATCH] D69897: Add #pragma clang loop aligned

hfinkel added inline comments. Comment at: clang/docs/LanguageExtensions.rst:3135 + +This predicates all the array references inside the loop to be aligned. The aligned access to them can increase fetch time and increase the performance. + hfinkel wrote: >

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

hfinkel added inline comments. Comment at: clang/lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h:17 #if defined(__NVPTX__) && defined(_OPENMP) Should we use a more-specific selector and then get rid of this `__NVPTX__` check?

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

hfinkel added a comment. In D71179#1774487 , @jdoerfert wrote: > In D71179#1774471 , @ABataev wrote: > > > They do this because they have several function definitions with the same > > name. In our case, we have

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

hfinkel added a comment. In D71179#1774678 , @ABataev wrote: > In D71179#1774487 , @jdoerfert wrote: > > > In D71179#1774471 , @ABataev wrote: > > > > > They do this because

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

hfinkel added a comment. In D71179#1775442 , @jdoerfert wrote: > > @jdoerfert , how does the ".ompvariant" work with external functions? I see > > the part of the spec which says, "The symbol name of a function definition > > that appears between a

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

hfinkel added a comment. In D71179#1775157 , @ABataev wrote: > In D71179#1775066 , @hfinkel wrote: > > > In D71179#1774678 , @ABataev wrote: > > > > > In D71179#1774487

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

hfinkel added a comment. In D71179#1775687 , @jdoerfert wrote: > ... > > >>> We restricted it for now to function definitions so we don't need to define >>> the mangling as you cannot expect linking. (I did this to get it in TR8 >>> while I figured

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

hfinkel added a comment. In D71179#1776761 , @ABataev wrote: > In D71179#1776528 , @hfinkel wrote: > > > In D71179#1776491 , @ABataev wrote: > > > > > In D71179#1776487

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

hfinkel added a comment. In D71241#1778134 , @ABataev wrote: > ... >> >> >>> Also, check how -ast-print works with your solution. It returns different >>> result than expected because you're transform the code too early. It is >>> incorrect

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

hfinkel added a comment. In D71241#1782779 , @ABataev wrote: > In D71241#1782742 , @hfinkel wrote: > > > In D71241#1782703 , @ABataev wrote: > > > > > > > > > > > ... > > >

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

hfinkel added a comment. In D71241#1782723 , @ABataev wrote: > I don't insist on function redefinition solution. You want to replace > functions - fine, but do this at the codegen, not in AST. Again, no one is replacing anything, and we're not mutating

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

hfinkel added a comment. In D71241#1782668 , @ABataev wrote: > ... >> While we talk a lot about what you think is bad about this solution it seems >> we ignore the problems in the current one. Let me summarize a few: >> >> - Take

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

hfinkel added a comment. In D71241#1782703 , @ABataev wrote: > ... >> >> >>> Given we have two implementations, each at different points in the >>> pipeline, it might be constructive to each write down why you each >>> choose

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

hfinkel added a comment. In D71241#1783444 , @ABataev wrote: > In D71241#1782586 , @hfinkel wrote: > > > In D71241#1782460 , > > @JonChesterfield wrote: > > > > > >

[PATCH] D69770: [APFloat] Add recoverable string parsing errors to APFloat

hfinkel accepted this revision. hfinkel added a comment. This revision is now accepted and ready to land. If we really want to be confident that this is robust, we should probably fuzz-test it. Regardless, this seems like a definite improvement. LGTM. CHANGES SINCE LAST ACTION

[PATCH] D67923: [TLI] Support for per-Function TLI that overrides available libfuncs

hfinkel accepted this revision. hfinkel added a comment. This revision is now accepted and ready to land. In D67923#1784015 , @tejohnson wrote: > Please take a look. This is now updated to reflect the commit of D71193 > ,

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

hfinkel added a comment. In D71241#1782614 , @ABataev wrote: > In D71241#1782551 , @hfinkel wrote: > > > In D71241#1779168 , > > @JonChesterfield wrote: > > > > > Lowering

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

hfinkel added a comment. In D71241#1778671 , @ABataev wrote: > There can be another one issue with this solution with inline assembly. I’m > not completely sure about it, will try to investigate it tomorrow. I suggest > to discuss this solution with

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

hfinkel added a comment. In D71241#1782430 , @JonChesterfield wrote: > In D71241#1782427 , @ABataev wrote: > > > In D71241#1782425 , > > @JonChesterfield wrote: > > > > >

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

hfinkel added a comment. In D71241#1782652 , @ABataev wrote: > In D71241#1782648 , @hfinkel wrote: > > > In D71241#1782614 , @ABataev wrote: > > > > > In D71241#1782551

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

hfinkel added a comment. In D71241#1787571 , @ABataev wrote: > In D71241#1787265 , @hfinkel wrote: > > > In D71241#1786959 , @jdoerfert > > wrote: > > > > > In

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

hfinkel added a comment. In D71241#1786959 , @jdoerfert wrote: > In D71241#1786530 , @ABataev wrote: > > > Most probably, we can use this solution without adding a new expression. > > `DeclRefExpr` class can

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

hfinkel added inline comments. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:51 + /// at the time, and location, the callback is invoked. + using FinalizeCallbackTy = std::function; + jdoerfert wrote: > ABataev wrote: > > jdoerfert wrote: > > >

<    1   2   3   4   >