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

hfinkel added a comment. In https://reviews.llvm.org/D37436#869467, @aaron.ballman wrote: > In https://reviews.llvm.org/D37436#869462, @hfinkel wrote: > > > I think that I misunderstood your concern. Let me see if I can summarize > > your position: You believe that, when GCC implements this synt

[PATCH] D37913: [OpenMP] Enable the existing nocudalib flag for OpenMP offloading toolchain.

hfinkel added a comment. Please add a test case. Repository: rL LLVM https://reviews.llvm.org/D37913 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37912: [OpenMP] Bugfix: output file name drops the absolute path where full path is needed.

hfinkel added inline comments. Comment at: lib/Driver/ToolChains/Cuda.cpp:442 +SmallString<256> Name = llvm::sys::path::relative_path(II.getFilename()); +SmallString<256> FullPath = llvm::sys::path::root_path(II.getFilename()); llvm::sys::path::replace_extension(Name

[PATCH] D37914: [OpenMP] Don't throw cudalib not found error if only front-end is required.

hfinkel added inline comments. Comment at: test/Driver/openmp-offload-gpu.c:140 + +/// Check that error is not thrown by toolchain when no cuda lib device is found when using -c -S. +/// Check that the flag is passed when -fopenmp-relocatable-target is used. Do

[PATCH] D38113: OpenCL: Assume functions are convergent

hfinkel added a comment. In https://reviews.llvm.org/D38113#877874, @Anastasia wrote: > The problem of adding this attribute conservatively for all functions is that > it prevents some optimizations to happen. I agree to commit this as a > temporary fix to guarantee correctness of generated cod

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

[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#1623903 , @ABataev wrote: > In D65835#1623883 , @hfinkel wrote: > > > In D65835#1623814 , @hfinkel wrote: > > > > > In D65835#1623772

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

[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] 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#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#1570609 , @rjmccall wrote: > In D64128#1569836 , @hfinkel wrote: > > > In D64128#1569817 , @rjmccall > > wrote: > > > > > The pointer/inte

[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#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] 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] 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] 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] 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] 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] 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) { +

[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] 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] 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] 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] 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] D46441: [clang][CodeGenCXX] Noalias attr for copy/move constructor arguments

hfinkel added a comment. Makes sense to me. Repository: rC Clang https://reviews.llvm.org/D46441 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

hfinkel added a comment. In https://reviews.llvm.org/D48808#1149534, @ABataev wrote: > I don't think that this is the intended behavior of the `#pragma clang loop`. > it is better to ask the author of this pragma is this correct or not. It is the intended behavior that the memory accesses are

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

hfinkel added a comment. In https://reviews.llvm.org/D48808#1149828, @Meinersbur wrote: > In https://reviews.llvm.org/D48808#1149549, @hfinkel wrote: > > > In https://reviews.llvm.org/D48808#1149534, @ABataev wrote: > > > > > > > > > > > Michael, can you please add a test with two inner loops, on

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

hfinkel added a comment. In https://reviews.llvm.org/D48808#1149862, @Meinersbur wrote: > In https://reviews.llvm.org/D48808#1149845, @hfinkel wrote: > > > We specifically defined the metadata to support nested loops. The LangRef > > says, "The llvm.mem.parallel_loop_access metadata refers to a

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

hfinkel added a comment. In https://reviews.llvm.org/D48721#1150333, @bjope wrote: > I tried running > > /clang -cc1 -O3 -funroll-loops -S -emit-llvm pragma-do-while-unroll.cpp -o > - -mllvm -print-after-all > > > > and I get this > > ... > !2 = distinct !{!2, !3} > !3 = !{!"llvm.loop.

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

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

[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] 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] 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] 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 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] 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] 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] 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] 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] 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] D39455: [CodeGen] Add initial support for union members in TBAA

hfinkel accepted this revision. hfinkel added a comment. This revision is now accepted and ready to land. I think this looks fine (and I agree that the underlying premise makes sense). Before you commit, could you also please add some tests with nested union members? Repository: rL LLVM htt

[PATCH] D39455: [CodeGen] Add initial support for union members in TBAA

hfinkel added a comment. In https://reviews.llvm.org/D39455#943182, @aheejin wrote: > Once you confirm the bug, could you possibly revert the patch? I agree. We should revert this. The relevant part of the test case is: short *q; p->u.vec[i] = 0; q = &p->u.vec[16]; *q = 1; return p->

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

hfinkel added a comment. I think this is a good idea. Comment at: include/clang/Driver/CC1Options.td:407 def verify : Flag<["-"], "verify">, - HelpText<"Verify diagnostic output using comment directives">; + HelpText<"Similar to -verify=expected">; def verify_ignore_unexpec

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

hfinkel added inline comments. Comment at: lib/Frontend/CompilerInvocation.cpp:1095 + if (Diags) { +Diags->Report(diag::err_drv_invalid_value) << "-verify=" << Prefix; +Diags->Report(diag::note_drv_verify_prefix_unique); jdenny wrote: > hfink

[PATCH] D39812: [Driver, CodeGen] pass through and apply -fassociative-math

hfinkel accepted this revision. hfinkel added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D39812 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe

[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] D35817: Ban implicit _Complex to scalar conversions in C++

hfinkel added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3068 +def err_impcast_complex_scalar : Error< + "implicit conversion discards imaginary component: %0 to %1">; def warn_impcast_float_precision : Warning< I think that,

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

hfinkel added a comment. In https://reviews.llvm.org/D32199#828526, @rjmccall wrote: > Has this proposal run aground? I'm going back over some old patches that > I've been CC'ed on and trying to make sure they're not blocking on my review. I need to rebase these now that we've "fixed" the uni

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

hfinkel added inline comments. Comment at: lib/Driver/ToolChain.cpp:808 + continue; + } else if (XOpenMPTargetNoTriple) +// Passing device args: -Xopenmp-target -opt=val. Please include {} around this else-if code, even though it is not nec

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

hfinkel accepted this revision. hfinkel added a comment. This revision is now accepted and ready to land. LGTM. Thanks for all of your work on this! Comment at: test/Driver/openmp-offload.c:603 + +/// Check -Xopenmp-target=powerpc64le-ibm-linux-gnu -march=pwr8 is passed when c

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

hfinkel added a comment. In https://reviews.llvm.org/D29654#835392, @gtbercea wrote: > In https://reviews.llvm.org/D29654#835371, @arphaman wrote: > > > The last RUN line in the new commit triggers the same assertion failure: > ... > Hi Alex, I'm not sure why it's failing as I can't reproduce

[PATCH] D35817: Ban implicit _Complex to scalar conversions in C++

hfinkel added a comment. What's going on with the OpenMP test changes? https://reviews.llvm.org/D35817 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35817: Ban implicit _Complex to scalar conversions in C++

hfinkel accepted this revision. hfinkel added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D35817#835740, @t.p.northover wrote: > > What's going on with the OpenMP test changes? > > Compound assignment operators like "real /= complex" become illegal unde

[PATCH] D36537: [OpenMP] Enable executable lookup into driver directory.

hfinkel accepted this revision. hfinkel added a comment. This revision is now accepted and ready to land. I think this is fine, but please add a comment explaining that you're doing this to find (e.g., to find the offload bundler, etc.). Repository: rL LLVM https://reviews.llvm.org/D36537

[PATCH] D34158: For Linux/gnu compatibility, preinclude if the file is available

hfinkel added a comment. In https://reviews.llvm.org/D34158#837130, @joerg wrote: > In https://reviews.llvm.org/D34158#836026, @jyknight wrote: > > > In https://reviews.llvm.org/D34158#827178, @joerg wrote: > > > > > (2) It adds magic behavior that can make debugging more difficult. > > > Partia

[PATCH] D29660: [OpenMP] Add flag for overwriting default PTX version for OpenMP targets

hfinkel added a comment. In https://reviews.llvm.org/D29660#838150, @gtbercea wrote: > First of all, I apologize if I've upset you with my previous post. I am > actively working on understanding what is causing these issues. It is not my > intention to write tests that work on local configurati

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

hfinkel updated this revision to Diff 09. 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] D9403: llvm.noalias - Clang CodeGen for local restrict-qualified pointers

hfinkel added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1291 +// where the pointer to the local variable is the key in the map. +void CodeGenFunction::EmitAutoVarNoAlias(const AutoVarEmission &emission) { + assert(emission.Variable && "emission was not valid!"); --

[PATCH] D9403: llvm.noalias - Clang CodeGen for local restrict-qualified pointers

hfinkel updated this revision to Diff 29. hfinkel added a comment. Herald added a subscriber: javed.absar. Rebased and addressing review comments. https://reviews.llvm.org/D9403 Files: lib/CodeGen/CGDecl.cpp lib/CodeGen/CGExpr.cpp lib/CodeGen/CGStmt.cpp lib/CodeGen/CodeGenFunction.c

[PATCH] D22189: llvm.noalias - Clang CodeGen - check restrict variable map only for restrict-qualified lvalues

hfinkel updated this revision to Diff 30. hfinkel added a comment. Herald added a subscriber: jholewinski. Rebased. https://reviews.llvm.org/D22189 Files: lib/CodeGen/CGDeclCXX.cpp lib/CodeGen/CGExpr.cpp lib/CodeGen/CGOpenMPRuntime.cpp lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp lib/Code

[PATCH] D37035: Implement __builtin_LINE() et. al. to support source location capture.

hfinkel added inline comments. Comment at: include/clang/AST/Expr.h:3814 +/// BuiltinSourceLocExpr - Represents a function call to one of +/// __builtin_LINE(), __builtin_COLUMN(), __builtin_FUNCTION(), or BuiltinSourceLocExpr -> SourceLocExpr ===

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

hfinkel added a comment. I'd really like to see this done in some way such that we can issue a warning along with generating well-defined code. The warning can specifically state that the code is using an extension, etc. https://reviews.llvm.org/D37042 __

[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] 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] 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/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 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] 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] 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] 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] 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] 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 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#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#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#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#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#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#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 behavior

[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#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] 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#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#1782614 , @ABataev wrote: > In D71241#1782551 , @hfinkel wrote: > > > In D71241#1779168 , > > @JonChesterfield wrote: > > > > > Lowering i

<    1   2   3   4   >