[PATCH] D83915: [PowerPC] Remove QPX/A2Q BGQ/BGP CNK support

hfinkel accepted this revision as: hfinkel. hfinkel added a comment. This revision is now accepted and ready to land. LGTM. Thank you. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83915/new/ https://reviews.llvm.org/D83915 _

[PATCH] D83915: [PowerPC] Remove QPX/A2Q BGQ/BGP support

hfinkel added a comment. Thanks for working on this. Please, however, take another pass of the test cases (especially those that are not in the PowerPC directory). Many of those should not be deleted, please change triple instead. They're testing general functionality. Comm

[PATCH] D77683: [Docs] Make code review policy clearer about requested pre-commit reviews

hfinkel added a comment. In D77683#1973762 , @dblaikie wrote: > In D77683#1973757 , @jdoerfert wrote: > > > In D77683#1970826 , @mehdi_amini > > wrote: > > > > > I am still

[PATCH] D75779: [OpenMP] `omp begin/end declare variant` - part 2, sema (+"CG")

hfinkel accepted this revision. hfinkel added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75779/new/ https://reviews.llvm.org/D75779 _

[PATCH] D75779: [OpenMP] `omp begin/end declare variant` - part 2, sema (+"CG")

hfinkel added inline comments. Comment at: clang/lib/AST/DeclarationName.cpp:147 +Name = Name.split(getOpenMPVariantManglingSeparatorStr()).first; + OS << Name; +} Would it make sense to print " (omp variant)" after the name to disambiguate? Re

[PATCH] D75779: [OpenMP] `omp begin/end declare variant` - part 2, sema (+"CG")

hfinkel added inline comments. Comment at: clang/lib/Sema/SemaOpenMP.cpp:5396 + IdentifierInfo &VariangtII = Context.Idents.get( + (D.getIdentifier()->getName() + "." + DVScope.NameSuffix).str()); + D.SetIdentifier(&VariangtII, D.getBeginLoc()); jdoerfert

[PATCH] D75779: [OpenMP] `omp begin/end declare variant` - part 2, sema (+"CG")

hfinkel added inline comments. Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1253 +: Error<"expected '#pragma omp begin declare variant' to match this stray " +"`#pragma omp end delcare variant`">; def err_omp_declare_target_unexpected_clause: Err

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

hfinkel requested changes to this revision. hfinkel added a comment. This revision now requires changes to proceed. Unfortunately, we cannot do this kind of thing just because it seems to make sense. The language semantics must be exactly satisfied by the IR-level semantics. I certainly agree th

[PATCH] D72996: [Sema] Attempt to perform call-size-specific `__attribute__((alloc_align(param_idx)))` validation

hfinkel added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:4489 +// Alignment calculations can wrap around if it's greater than 2**29. +unsigned MaximumAlignment = 536870912; +if (I > MaximumAlignment) jdoerfert wrote: > hf

[PATCH] D72996: [Sema] Attempt to perform call-size-specific `__attribute__((alloc_align(param_idx)))` validation

hfinkel added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:4489 +// Alignment calculations can wrap around if it's greater than 2**29. +unsigned MaximumAlignment = 536870912; +if (I > MaximumAlignment) jdoerfert wrote: > er

[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

hfinkel added a comment. > I'm not sure whether this is deliberate (but it seems weird) or just a bug. I > can ask the GCC developers ... Please do. If there's a rationale, we should know. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72675/new/ https://reviews.llvm.org/D72675

[PATCH] D72998: [IR] Attribute/AttrBuilder: use Value::MaximumAlignment magic constant

hfinkel added a comment. Can we, at least, put this constant in a header file so we don't repeat it in several places? Also, can we write it as 0x2000 so that it's more obvious what the value is. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D7

[PATCH] D72675: ix -ffast-math/-ffp-contract interaction

hfinkel added inline comments. Herald added a subscriber: wuzish. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2721 if (!MathErrno && AssociativeMath && ReciprocalMath && !SignedZeros && - !TrappingMath) + !TrappingMath && !FPContractDisabled) CmdArgs.push_

[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 D71241#178653

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

[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] 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 https://reviews

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

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

[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 https://godbolt.org/z

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

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

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

hfinkel added a comment. In D71241#1779779 , @ABataev wrote: > Here is the example that does not work with the proposed solution but works > with the existing one: > > static void cpu() { asm("nop"); } > > #pragma omp declare variant(cpu) match(devi

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

hfinkel added a comment. In D71241#1782460 , @JonChesterfield wrote: > > https://clang.llvm.org/docs/InternalsManual.html#the-ast-library > > > > Faithfulness¶ > > The AST intends to provide a representation of the program that is > > faithful to th

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

hfinkel added a comment. In D71241#1779168 , @JonChesterfield wrote: > Lowering in sema or in codegen seems a standard phase ordering choice. There > will be pros and cons to both. > > I think prior art leans towards sema. Variants are loosely equivalent

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

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

[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] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

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

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

hfinkel added a comment. In D71179#1776467 , @ABataev wrote: > In D71179#1776457 , @jdoerfert wrote: > > > > You're doing absolutely the same thing as the original declare variant > > > implementation. > > > > I do

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

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

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

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

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

[PATCH] D69391: Add #pragma clang loop ivdep

hfinkel added a comment. This is the same as #pragma clang loop vectorize(assume_safety)? In D69391#1720845 , @xbolva00 wrote: > "#pragma ivdep" should work too (compatibiluty mode with intel, gcc). The semantics are not the same, unfortunately. Repos

[PATCH] D69272: Restricted variant of '#pragma STDC FENV_ACCESS'

hfinkel added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:890 + "cannot apply to inline functions, ignoring pragma">, + InGroup; sepavloff wrote: > hfinkel wrote: > > sepavloff wrote: > > > hfinkel wrote: > > > > andrew.w.ka

[PATCH] D69272: Restricted variant of '#pragma STDC FENV_ACCESS'

hfinkel added a comment. > I don't see how such warning can help a user. A note about impact of the > pragma on performance can be put into documentation. Issuing a warning on > every use of the pragma may be annoying. I definitely agree. Performance may be fine in many cases, and performance m

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

hfinkel accepted this revision. hfinkel added a comment. This revision is now accepted and ready to land. In D63607#1709258 , @peterwaller-arm wrote: > Friendly ping to everybody watching. I'd like to get this in soon if possible. > > Hal - do you think t

[PATCH] D69272: Restricted variant of '#pragma STDC FENV_ACCESS'

hfinkel added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:890 + "cannot apply to inline functions, ignoring pragma">, + InGroup; sepavloff wrote: > hfinkel wrote: > > andrew.w.kaylor wrote: > > > rjmccall wrote: > > > > What

[PATCH] D69272: Restricted variant of '#pragma STDC FENV_ACCESS'

hfinkel added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:890 + "cannot apply to inline functions, ignoring pragma">, + InGroup; andrew.w.kaylor wrote: > rjmccall wrote: > > What's the purpose of this restriction? Whether `

[PATCH] D68824: Fix __builtin_assume_aligned with too large values.

hfinkel added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2854 +def warn_assume_align_too_big : Warning<"alignments greater than 1 << 29 are " + "unsupported by LLVM">, InGroup; Should this be in the IgnoredAttributes group? T

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

hfinkel added a comment. In D64943#1683368 , @JonChesterfield wrote: > The three way split looks great, thanks. Makes sense to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64943/new/ https://reviews.llvm.org/D64943 ___

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

hfinkel added a comment. This LGTM. I'm happy that this is a design improvement over the current scheme. @JonChesterfield , @ABataev , any further comments? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64943/new/ https://reviews.llvm.org/D64943 _

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

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

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

[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] 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 we'r

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

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

[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 https://r

[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] 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: %clan

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

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

[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 https://review

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

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

[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 , @efriedm

[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] 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] 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 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] 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] 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] 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] 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#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 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] 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] 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#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] 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] 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 added a comment. In D59712#1472318 , @jdenny wrote: > In D59712#1469760 , @lebedev.ri > wrote: > > > In D59712#1469693 , @jdenny wrote: > > > > > In D59712#1469392 <

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

[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 &AL : Active) { +

  1   2   3   4   >