[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#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#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#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#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#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#1783444 , @ABataev wrote: > In D71241#1782586 , @hfinkel wrote: > > > In D71241#1782460 , > > @JonChesterfield wrote: > > > > > > https://

[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] 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#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] 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] 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: [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] 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] 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] D24933: Enable configuration files in clang

hfinkel added inline comments. Comment at: lib/Driver/Driver.cpp:630 + for (const char *Dir : Directories) { +assert(Dir); +FilePath.clear(); assert(Dir && "Directory path should not be null"); Comment at: lib/Driver/Driver.cpp:637 +

[PATCH] D24933: Enable configuration files in clang

hfinkel added inline comments. Comment at: lib/Driver/Driver.cpp:661 + + // If not found, try searching the directory where executable resides. + FilePath.clear(); hfinkel wrote: > Why not just add this directory to the end of the list of directories? (by which

[PATCH] D39331: Add flags to control the -finstrument-functions instrumentation calls to __cyg_profile functions

hfinkel added a comment. Quick question: We're talking about a few different variants on this feature now: Post-inlining instrumentation, only marking function entries, and not passing the function address. Do you intend to use all of these things separately? I ask because, taken together, that

[PATCH] D38824: [X86] Synchronize the existing CPU predefined macros with the cases that gcc defines them

hfinkel added a comment. In https://reviews.llvm.org/D38824#908461, @chandlerc wrote: > So, doing research to understand the impact of this has convinced me we > *really* need to stop doing this. Multiple libraries are actually trying to > enumerate every CPU that has feature X for some feature

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

hfinkel added a comment. In https://reviews.llvm.org/D39053#906513, @spetrovic wrote: > Well, basically I'm just expanding the existing algorithm, why should we > split fields just in case when current field is integer, > I'm not resolving specific problem with unaligned loads/stores on MIPS. >

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

hfinkel added a comment. In https://reviews.llvm.org/D39204#905860, @efriedma wrote: > I think you're understanding the semantics correctly. > > For r265521, look again at the implementation of > llvm::checkUnaryFloatSignature; if "I.onlyReadsMemory()" is true, we somehow > proved the call does

[PATCH] D24933: Enable configuration files in clang

hfinkel added inline comments. Comment at: lib/Driver/Driver.cpp:706 + + // Determine architecture part of the file name, if it presents. + size_t ArchPrefixLen = 0; if it presents. -> if it is present. Comment at: lib/Driver/Driver.cpp:739 +

[PATCH] D39376: [PowerPC] Add implementation for -msave-toc-indirect option - clang portion

hfinkel added a comment. Please upload diffs with full context. Comment at: include/clang/Driver/Options.td:1907 def mvsx : Flag<["-"], "mvsx">, Group; +def msave_toc_indirect : Flag<["-"], "msave-toc-indirect">, Group; def mno_vsx : Flag<["-"], "mno-vsx">, Group; --

[PATCH] D39481: [CodeGen] fix const-ness of builtin equivalents of and functions that might set errno

hfinkel added a comment. In https://reviews.llvm.org/D39481#914174, @craig.topper wrote: > There's an oddity with fma. The version without __builtin has 'e' already Something that is potentially relevant: POSIX says that fma() can set errno (http://pubs.opengroup.org/onlinepubs/9699919799/func

[PATCH] D39611: [CodeGen] make cbrt and fma constant (never set errno); document complex calls as always constant

hfinkel added a comment. In https://reviews.llvm.org/D39611#915560, @efriedma wrote: > > The POSIX docs all have language like this for complex calls: > > "No errors are defined." > > I would not trust the POSIX documentation. > > carg() is an alias for atan2(), and cabs() is an alias for hypot(

[PATCH] D39611: [CodeGen] make cbrt and fma constant (never set errno); document complex calls as always constant

hfinkel added a comment. In https://reviews.llvm.org/D39611#915799, @efriedma wrote: > > In general, because all of these functions are well defined over the entire > > complex domain, they don't have errors. The "no errors defined" is likely > > correct. > > One, it is not true that all these

[PATCH] D39611: [CodeGen] change const-ness of complex calls

hfinkel added a comment. In https://reviews.llvm.org/D39611#918275, @spatel wrote: > Patch updated: > I don't know if we have agreement on the behavior that we want yet, I just sent a note to the Austin group mailing list to see if the POSIX folks agree with my reading. I'll follow up. > but

[PATCH] D39611: [CodeGen] change const-ness of complex calls

hfinkel added a comment. In https://reviews.llvm.org/D39611#918654, @hfinkel wrote: > In https://reviews.llvm.org/D39611#918275, @spatel wrote: > > > Patch updated: > > I don't know if we have agreement on the behavior that we want yet, > > > I just sent a note to the Austin group mailing list t

[PATCH] D39611: [CodeGen] change const-ness of complex calls

hfinkel added a comment. In https://reviews.llvm.org/D39611#921923, @spatel wrote: > Thanks for the clarification! > > If I'm reading this properly, we should make the same kind of change as in > https://reviews.llvm.org/D39481 ('c' -> 'e') for most of complex.h. Ie, the > standard allows errno

[PATCH] D40144: Implement `std::launder`

hfinkel added a comment. In https://reviews.llvm.org/D40144#927828, @tcanens wrote: > At least for GCC, it should use `__builtin_launder`. I presume we'll need to add something similar for Clang as well. > Also needs to implement "The program is ill-formed if `T` is a function type > or //cv/

[PATCH] D40275: [CUDA] Report "unsupported VLA" errors only on device side.

hfinkel added a comment. > When Sema sees this code during compilation, it can not tell whether there is > an error. Calling foo from the host code is perfectly valid. Calling it from > device code is not. CUDADiagIfDeviceCode creates 'postponed' diagnostics > which only gets emitted if we ever

[PATCH] D23934: Add a -ffixed-date-time= flag that sets the initial value of __DATE__, __TIME__, __TIMESTAMP__

hfinkel added a comment. In https://reviews.llvm.org/D23934#631656, @ed wrote: > I'd be interested in seeing a feature like this appearing. I agree. Comment at: lib/Driver/Tools.cpp:4687 +CmdArgs.push_back(Args.MakeArgString("-ffixed-date-time=" + DateTime)); + } +

[PATCH] D23934: Add a -ffixed-date-time= flag that sets the initial value of __DATE__, __TIME__, __TIMESTAMP__

hfinkel added a comment. In https://reviews.llvm.org/D23934#632216, @emaste wrote: > Please also accept `SOURCE_DATE_EPOCH` set in the environment -- see > https://reproducible-builds.org/specs/source-date-epoch/ It looks like there's reasonable adoption of this: https://wiki.debian.org/Repro

[PATCH] D27872: [APFloat] Switch from (PPCDoubleDoubleImpl, IEEEdouble) layout to (IEEEdouble, IEEEdouble)

hfinkel added a comment. > (IEEEdouble, IEEEdouble) -> (uint64_t, uint64_t) -> PPCDoubleDoubleImpl, then > run the old algorithm. We need to document in the code what is going on here and why it works. Just looking at a bunch of code like this: if (Semantics == &semPPCDoubleDouble) { APF

[PATCH] D24688: Introduce "hosted" module flag.

hfinkel added a comment. In https://reviews.llvm.org/D24688#639158, @jyknight wrote: > Since this requires everyone generating llvm IR to add this module attribute > for optimal codegen, it should be documented in release notes and the > LangRef, I think. And, for something like this, here:

[PATCH] D28213: [Frontend] Correct values of ATOMIC_*_LOCK_FREE to match builtin

hfinkel accepted this revision. hfinkel added a comment. This revision is now accepted and ready to land. LGTM. This seems consistent with what GCC does. On x86 it also sets __GCC_ATOMIC_LLONG_LOCK_FREE to 2. https://reviews.llvm.org/D28213 ___ cfe

[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values

hfinkel added a comment. In https://reviews.llvm.org/D30923#700696, @rnk wrote: > In https://reviews.llvm.org/D30923#700626, @hfinkel wrote: > > > In https://reviews.llvm.org/D30923#700620, @thakis wrote: > > > > > Maybe it should have some "to suppress the warning, do X" fixit? > > > > > > I thi

[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values

hfinkel added a comment. In https://reviews.llvm.org/D30923#700780, @rnk wrote: > In https://reviews.llvm.org/D30923#700708, @hfinkel wrote: > > > In https://reviews.llvm.org/D30923#700696, @rnk wrote: > > > > > Do you think it's worth indicating that the error can be suppressed with > > > an ex

[PATCH] D30898: Add new -fverbose-asm that enables source interleaving

hfinkel added a comment. > Note that in contrast to the original suggestion -fsource-asm here we use the > preferred -fverbose-asm. Basically explicitly saying -fverbose-asm in the > command line enables a minimum amount of debugging, so in AsmPrinter we can > use it to print the source code.

[PATCH] D30920: Do not pass -Os and -Oz to the Gold plugin

hfinkel added a comment. In https://reviews.llvm.org/D30920#700741, @mehdi_amini wrote: > In https://reviews.llvm.org/D30920#700574, @hfinkel wrote: > > > In https://reviews.llvm.org/D30920#700557, @mehdi_amini wrote: > > > > > In https://reviews.llvm.org/D30920#700433, @tejohnson wrote: > > > >

[PATCH] D30920: Do not pass -Os and -Oz to the Gold plugin

hfinkel added a comment. In https://reviews.llvm.org/D30920#703083, @mehdi_amini wrote: > In https://reviews.llvm.org/D30920#703082, @hfinkel wrote: > > > In https://reviews.llvm.org/D30920#700741, @mehdi_amini wrote: > > > > > Yes, the issue is only about how the driver accepts Os for the link e

[PATCH] D30415: Fix -mno-altivec cannot overwrite -maltivec option

hfinkel added a comment. In https://reviews.llvm.org/D30415#703398, @echristo wrote: > Different suggestion: > > Remove the faltivec option. Even gcc doesn't support it anymore afaict. What are you suggesting? Always having the language extensions on? Or explicitly tying the language extension

[PATCH] D30806: [nonnull] Teach Clang to attach the nonnull LLVM attribute to declarations and calls instead of just definitions, and then teach it to *not* attach such attributes even if the source c

hfinkel added inline comments. Comment at: include/clang/AST/ASTContext.h:1865 /// arguments to the builtin that are required to be integer constant /// expressions. QualType GetBuiltinType(unsigned ID, GetBuiltinTypeError &Error, Please add some descrip

[PATCH] D30806: [nonnull] Teach Clang to attach the nonnull LLVM attribute to declarations and calls instead of just definitions, and then teach it to *not* attach such attributes even if the source c

hfinkel added inline comments. Comment at: include/clang/AST/ASTContext.h:1868 + bool *OverrideNonnullReturn = nullptr, + unsigned *OverrideNonnullArgs = nullptr, unsigned *IntegerConstantArgs = nullptr)

[PATCH] D30806: [nonnull] Teach Clang to attach the nonnull LLVM attribute to declarations and calls instead of just definitions, and then teach it to *not* attach such attributes even if the source c

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

[PATCH] D31276: Add #pragma clang fast_math

hfinkel added a comment. High-level comment ;) #pragma clang fast_math contract_fast(on) This seems a bit unfortunate because 'fast' appears twice? How are we planning on naming the other fast-math flags? Maybe we should just name it: #pragma clang math constract_fast(on) or #pragma cl

[PATCH] D31276: Add #pragma clang fast_math

hfinkel added a comment. In https://reviews.llvm.org/D31276#708993, @anemet wrote: > In https://reviews.llvm.org/D31276#708976, @aaron.ballman wrote: > > > In https://reviews.llvm.org/D31276#708968, @anemet wrote: > > > > > Thanks very much, Aaron! It would be great if you could also look at the

[PATCH] D30920: Do not pass -Os and -Oz to the Gold plugin

hfinkel added a comment. In https://reviews.llvm.org/D30920#703305, @mehdi_amini wrote: > The fundamental difference, is that Os/Oz especially are treated as > `optimizations directive` that are independent of the pass pipeline: > instructing that "loop unroll should not increase size" is indep

[PATCH] D30920: Do not pass -Os and -Oz to the Gold plugin

hfinkel added a comment. In https://reviews.llvm.org/D30920#710329, @mehdi_amini wrote: > In https://reviews.llvm.org/D30920#710209, @hfinkel wrote: > > > Yes, we should do this. I don't understand why this is tricky. Actually, I > > think having these kinds of decisions explicit in the code of

[PATCH] D31276: Add #pragma clang fast_math

hfinkel added a comment. In https://reviews.llvm.org/D31276#709111, @anemet wrote: > In https://reviews.llvm.org/D31276#708992, @hfinkel wrote: > > > High-level comment ;) > > > > #pragma clang fast_math contract_fast(on) > > > > > > This seems a bit unfortunate because 'fast' appears twice?

[PATCH] D31561: cmath: Skip Libc for integral types in isinf, etc.

hfinkel added inline comments. Comment at: libcxx/include/math.h:354 inline _LIBCPP_INLINE_VISIBILITY -typename std::enable_if::value, bool>::type +typename std::enable_if::value, bool>::type signbit(_A1 __lcpp_x) _NOEXCEPT I think that it would be safer / easi

[PATCH] D31167: Use FPContractModeKind universally

hfinkel added inline comments. Comment at: cfe/trunk/lib/Frontend/CompilerInvocation.cpp:1638 Opts.LaxVectorConversions = 0; -Opts.DefaultFPContract = 1; +Opts.setDefaultFPContractMode(LangOptions::FPC_On); Opts.NativeHalfType = 1; Looks like th

[PATCH] D31167: Use FPContractModeKind universally

hfinkel added inline comments. Comment at: cfe/trunk/lib/Frontend/CompilerInvocation.cpp:1638 Opts.LaxVectorConversions = 0; -Opts.DefaultFPContract = 1; +Opts.setDefaultFPContractMode(LangOptions::FPC_On); Opts.NativeHalfType = 1; yaxunl wrote:

[PATCH] D31561: cmath: Skip Libc for integral types in isinf, etc.

hfinkel added a subscriber: mclow.lists. hfinkel added a comment. I'm happy with this now. @EricWF , @mclow.lists , thoughts? https://reviews.llvm.org/D31561 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailm

[PATCH] D31413: [libc++] Use __attribute__((init_priority(101))) to ensure streams get initialized early

hfinkel added a comment. In https://reviews.llvm.org/D31413#712013, @aaron.ballman wrote: > I'm not certain of a good way to test it, but I have a question about the > value you picked for `init_priority`. My understanding of the values starting > from 101 is that 1-100 are reserved for impleme

[PATCH] D31167: Use FPContractModeKind universally

hfinkel added inline comments. Comment at: cfe/trunk/include/clang/Basic/LangOptions.def:220 +/// \brief FP_CONTRACT mode (on/off/fast). +ENUM_LANGOPT(DefaultFPContractMode, FPContractModeKind, 2, FPC_Off, "FP contraction type") LANGOPT(NoBitFieldTypeAlign , 1, 0, "bit-field ty

[PATCH] D31167: Use FPContractModeKind universally

hfinkel added inline comments. Comment at: cfe/trunk/include/clang/Basic/LangOptions.def:220 +/// \brief FP_CONTRACT mode (on/off/fast). +ENUM_LANGOPT(DefaultFPContractMode, FPContractModeKind, 2, FPC_Off, "FP contraction type") LANGOPT(NoBitFieldTypeAlign , 1, 0, "bit-field ty

[PATCH] D31167: Use FPContractModeKind universally

hfinkel added inline comments. Comment at: cfe/trunk/include/clang/Basic/LangOptions.def:220 +/// \brief FP_CONTRACT mode (on/off/fast). +ENUM_LANGOPT(DefaultFPContractMode, FPContractModeKind, 2, FPC_Off, "FP contraction type") LANGOPT(NoBitFieldTypeAlign , 1, 0, "bit-field ty

[PATCH] D31885: Remove TBAA information from LValues representing union members

hfinkel added a comment. I'm not sure this is the right way to do this; I suspect we're lumping together a bunch of different bugs: 1. vector types need to have tbaa which makes them alias with their element types [to be clear, as vector types are an implementation extension, this is our choic

[PATCH] D31167: Use FPContractModeKind universally

hfinkel added inline comments. Comment at: cfe/trunk/include/clang/Basic/LangOptions.def:220 +/// \brief FP_CONTRACT mode (on/off/fast). +ENUM_LANGOPT(DefaultFPContractMode, FPContractModeKind, 2, FPC_Off, "FP contraction type") LANGOPT(NoBitFieldTypeAlign , 1, 0, "bit-field ty

[PATCH] D31885: Remove TBAA information from LValues representing union members

hfinkel added a comment. In https://reviews.llvm.org/D31885#727307, @kparzysz wrote: > This is not meant as a fine-grained solution to the TBAA problem, but a > temporary fix for generating wrong information. Just yesterday I helped > diagnose yet another problem related to this, so this issue

[PATCH] D31885: Remove TBAA information from LValues representing union members

hfinkel added a comment. > I'm not sure about the solution to #2, because i thought there were very > specific points in time at which the effective type could change. I think this is a key point. I'm not sure that there are specific points that the frontend can deduce: union U { int i;

[PATCH] D31885: Remove TBAA information from LValues representing union members

hfinkel added a comment. In https://reviews.llvm.org/D31885#727371, @efriedma wrote: > In https://reviews.llvm.org/D31885#727167, @hfinkel wrote: > > > I'm not sure this is the right way to do this; I suspect we're lumping > > together a bunch of different bugs: > > > > 1. vector types need to h

[PATCH] D32199: [TBAASan] A TBAA Sanitizer (Clang)

hfinkel created this revision. Herald added subscribers: mcrosier, emaste. This patch introduces the runtime components of a TBAA sanitizer: a sanitizer for type-based aliasing violations. C/C++ have type-based aliasing rules, and LLVM's optimizer can exploit these given TBAA metadata added by

[PATCH] D31885: Remove TBAA information from LValues representing union members

hfinkel added a comment. To be clear, I'm find with disabling union tbaa until we can fix things for real. I'm somewhat concerned that this patch is quadratic in the AST. FWIW, I tried just setting TBAAPath = true in EmitLValueForField and then returning nullptr from CodeGenTBAA::getTBAAStructT

[PATCH] D28845: Prototype of modules codegen

hfinkel added a comment. Can you provide a high-level description of what you're trying to accomplish and the usage model? https://reviews.llvm.org/D28845 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman

[PATCH] D24933: Enable configuration files in clang

hfinkel added inline comments. Comment at: lib/Driver/Driver.cpp:172 +NumConfigArgs = static_cast(NumCfgArgs); + } + sepavloff wrote: > bruno wrote: > > If `NumCfgArgs == 0` it would be nice to warn that the config file is empty? > Not sure if it makes sense

[PATCH] D24933: Enable configuration files in clang

hfinkel added inline comments. Comment at: include/clang/Driver/Driver.h:219 + /// This number may be smaller that \c NumConfigOptions if some options + /// requires separate arguments. + unsigned NumConfigArgs; requires -> require Comment a

[PATCH] D28543: Elliminates uninitialized warning for volitile varibles.

hfinkel added a comment. What's the motivation for this? The placement of a local volatile variable is still under the compiler's direction, and unless the address escapes, we still assume we can reason about its aliasing (and, thus, whether or not it is initialized). https://reviews.llvm.org

[PATCH] D29750: [PPC] Enable -fomit-frame-pointer by default for PPC

hfinkel added a comment. We should have regression tests for this. Maybe update test/Driver/frame-pointer-elim.c? https://reviews.llvm.org/D29750 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo

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

hfinkel added inline comments. Comment at: lib/Driver/ToolChains/Cuda.cpp:435 + +// TODO: get the compute capability from offloading arguments when not +// using the default compute capability of sm_20. gtbercea wrote: > hfinkel wrote: > > Why is this a T

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

hfinkel added inline comments. Comment at: lib/Driver/ToolChains/Cuda.cpp:435 + +// TODO: get the compute capability from offloading arguments when not +// using the default compute capability of sm_20. gtbercea wrote: > hfinkel wrote: > > gtbercea wrote:

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

hfinkel added inline comments. Comment at: test/Driver/openmp-offload.c:614 +/// Check -march propagates compute capability to device offloading toolchain. +// RUN: %clang -### -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -save-temps -no-canonical-prefixes -march=sm_35

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

hfinkel added inline comments. Comment at: test/Driver/openmp-offload.c:614 +/// Check -march propagates compute capability to device offloading toolchain. +// RUN: %clang -### -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -save-temps -no-canonical-prefixes -march=sm_35

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

hfinkel accepted this revision. hfinkel added a comment. This revision is now accepted and ready to land. LGTM Repository: rL LLVM https://reviews.llvm.org/D29647 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-b

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

hfinkel added a comment. In https://reviews.llvm.org/D29647#795271, @hfinkel wrote: > LGTM When you commit this, please make sure to mention in the commit message that the test cases will be associated with follow-up commits. Repository: rL LLVM https://reviews.llvm.org/D29647

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

hfinkel added a comment. What happens if you have multiple targets? Maybe this should be -fopenmp-targets-arch=foo,bar,whatever? Once this all lands, please make sure that you add additional test cases here. Make sure that the arch is passed through to the ptx and cuda tools as it should be. M

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

hfinkel added a comment. In https://reviews.llvm.org/D34784#795353, @gtbercea wrote: > In https://reviews.llvm.org/D34784#795287, @hfinkel wrote: > > > What happens if you have multiple targets? Maybe this should be > > -fopenmp-targets-arch=foo,bar,whatever? > > > > Once this all lands, please

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

hfinkel added a comment. In https://reviews.llvm.org/D34784#795871, @gtbercea wrote: > In https://reviews.llvm.org/D34784#795367, @hfinkel wrote: > > > In https://reviews.llvm.org/D34784#795353, @gtbercea wrote: > > > > > In https://reviews.llvm.org/D34784#795287, @hfinkel wrote: > > > > > > > Wh

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

hfinkel added a comment. In https://reviews.llvm.org/D34784#795980, @gtbercea wrote: > In https://reviews.llvm.org/D34784#795934, @hfinkel wrote: > > > In https://reviews.llvm.org/D34784#795871, @gtbercea wrote: > > > > > In https://reviews.llvm.org/D34784#795367, @hfinkel wrote: > > > > > > > In

[PATCH] D34846: Remove Clang support for '-fvectorize-slp-aggressive' which used LLVM's basic block vectorizer. This vectorizer has had no known users for many, many years and is completely surpassed

hfinkel accepted this revision. hfinkel added a comment. LGTM, but you need to get the: def fslp_vectorize_aggressive : Flag<["-"], "fslp-vectorize-aggressive">, Group, HelpText<"Enable the BB vectorization passes">; def fno_slp_vectorize_aggressive : Flag<["-"], "fno-slp-vectorize-aggr

[PATCH] D34846: Remove Clang support for '-fvectorize-slp-aggressive' which used LLVM's basic block vectorizer. This vectorizer has had no known users for many, many years and is completely surpassed

hfinkel added a comment. In https://reviews.llvm.org/D34846#796102, @rsmith wrote: > Removing a `-cc1` flag is OK: we explicitly tell people that the `-cc1` > interface is subject to change without notice. I'm assuming we can remove the regular flag as well. They'll just get an "unused argume

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

hfinkel added a comment. ... > Okay, good, this is exactly where I was going when I said I was worried about generalization. -march seems like one of many flags I might want to pass to the target compilation. Moreover, it doesn't seem special in what regard. >>

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

hfinkel added inline comments. Comment at: include/clang/Driver/Options.td:453 +def Xopenmp_target : Separate<["-"], "Xopenmp-target">, + HelpText<"Pass arguments to target offloading toolchain.">; +def Xopenmp_target_EQ : JoinedAndSeparate<["-"], "Xopenmp-target=">, ---

[PATCH] D34928: Add docs for -foptimization-record-file=

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

[PATCH] D34926: Deprecation flag group + use for BB vectorizer options

hfinkel accepted this revision. hfinkel added a comment. This revision is now accepted and ready to land. LGTM Comment at: docs/ReleaseNotes.rst:69 + +- -fslp-vectorize-aggressive used to enable the BB vectorizing passes. They have been superseeded + by the normal SLP vectori

[PATCH] D34578: cmath: Support clang's -fdelayed-template-parsing

hfinkel accepted this revision. hfinkel added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D34578#801357, @dexonsmith wrote: > Ping! Hal, you committed r283051. Do you have a problem with this? It looks like you're just renaming `__libcpp_isnan` and

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

hfinkel added inline comments. Comment at: lib/Driver/ToolChains/Cuda.cpp:474 +for (StringRef Opt : OptList) { + AddMArchOption(DAL, Opts, Opt); +} gtbercea wrote: > hfinkel wrote: > > Shouldn't you be adding all of the options, not just the -march=

[PATCH] D34158: For standards compatibility, preinclude if the file is available

hfinkel added a comment. In https://reviews.llvm.org/D34158#803752, @jyknight wrote: > This version is still disabling upon -nostdinc, which doesn't make sense to > me. I agree. If I pass -nostdinc, so that I then arrange include paths for my own libc headers, I'd then like to pick up the pre

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

hfinkel added inline comments. Comment at: lib/Driver/ToolChains/Cuda.cpp:474 +for (StringRef Opt : OptList) { + AddMArchOption(DAL, Opts, Opt); +} gtbercea wrote: > hfinkel wrote: > > gtbercea wrote: > > > hfinkel wrote: > > > > Shouldn't you be add

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

hfinkel added a comment. This is much closer to what I had in mind, thanks! Now I think we're in a position to make this work for more than just the CUDA target. It looks like the added code is now: 1. Remove -march from the translated arguments (because any existing -march would apply only fo

[PATCH] D28404: IRGen: Add optnone attribute on function during O0

hfinkel added a comment. In https://reviews.llvm.org/D28404#673260, @mehdi_amini wrote: > Ping :) To clarify my understanding of this thread, it seems like there are three ways forward here: 1. To have -O0 add optnone to the generated functions (enabling some degree of lack of optimization o

[PATCH] D30009: Add support for '#pragma clang attribute'

hfinkel added a comment. I don't understand why it only supports some attributes. Is there some handling that needs to take place (I don't see anything obvious in this patch)? If most attributes will "just work", I'd much rather opt-out the few exceptions (which we can then explicitly document)

[PATCH] D29778: Declare lgamma library builtins as never being const

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

[PATCH] D30009: Add support for '#pragma clang attribute'

hfinkel added a comment. In https://reviews.llvm.org/D30009#678890, @arphaman wrote: > In https://reviews.llvm.org/D30009#678091, @hfinkel wrote: > > > I don't understand why it only supports some attributes. Is there some > > handling that needs to take place (I don't see anything obvious in th

[PATCH] D29750: [PPC] Enable -fomit-frame-pointer by default for PPC

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

[PATCH] D30087: [Driver] Unify linking of OpenMP runtime

hfinkel added inline comments. Comment at: lib/Driver/Tools.cpp:10334 - if (Args.hasFlag(options::OPT_fopenmp, options::OPT_fopenmp_EQ, - options::OPT_fno_openmp, false)) { + // FIXME: Exclude this for platforms with libgomp that don't require +

[PATCH] D30538: Add documentation for -fno-strict-aliasing

hfinkel added inline comments. Comment at: docs/UsersManual.rst:1106 + + NOTE: unlike gcc, clang does not allow "type-punning" by writing and + reading from different union members without `-fno-strict-aliasing` Capitalize Unlike. https://reviews.llvm.org/D

[PATCH] D12689: [libc++][static linking] std streams are not initialized prior to their use in static object constructors

hfinkel added a comment. In https://reviews.llvm.org/D12689#515120, @eastig wrote: > The issue is reported as: > https://llvm.org/bugs/show_bug.cgi?id=28954 We do need to fix this bug; I posted to the PR so we can discuss approach there. https://reviews.llvm.org/D12689 ___

<    1   2   3   4   >