[PATCH] D50477: WIP: Ensure that the type of size_t is represended as one of the fixed width types

joerg added a comment. I don't understand the desire for this logic. Why can't wasm override the rest of the types if it wants to have something special? Repository: rC Clang https://reviews.llvm.org/D50477 ___ cfe-commits mailing list cfe-commi

[PATCH] D50839: [llvm] Optimize YAML::isNumeric

joerg added inline comments. Comment at: llvm/include/llvm/Support/YAMLTraits.h:477 + if (ParseOct && std::find(std::begin(OctalChars), std::end(OctalChars), +Char) == std::end(OctalChars)) +return false; Can you use s

[PATCH] D50839: [llvm] Optimize YAML::isNumeric

joerg added inline comments. Comment at: llvm/tools/llvm-yaml-numeric-parser-fuzzer/yaml-numeric-parser-fuzzer.cpp:16 + +llvm::Regex Inifnity("^[-+]?(\\.inf|\\.Inf|\\.INF)$"); +llvm::Regex Base8("^0o[0-7]+$"); Spelling? https://reviews.llvm.org/D50839 _

[PATCH] D50966: Fix an undefined behavior when storing an empty StringRef.

joerg added a comment. Why do we need to allocate memory in this case at all? I.e. why can't this just be: if (S.empty()) return StringRef("", 0); ... Repository: rL LLVM https://reviews.llvm.org/D50966 ___ cfe-commits mailing list cfe-

[PATCH] D50413: [libunwind][include] Add some missing definitions to .

joerg added a comment. Is there a reason for defining them? As in: does anything outside libunwind use them? I haven't seen such software yet. Repository: rUNW libunwind https://reviews.llvm.org/D50413 ___ cfe-commits mailing list cfe-commits@li

[PATCH] D47138: [Sparc] Use the leon arch for Leon3's when using an external assembler

joerg added a comment. This still needs a test case? https://reviews.llvm.org/D47138 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38861: [CodeGen] Error on unsupported checked multiplies early

joerg added inline comments. Comment at: lib/CodeGen/CGBuiltin.cpp:2263 + } +} + vsk wrote: > rjmccall wrote: > > Is there a reason this only fails on x86? If LLVM doesn't have generic > > wide-operation lowering, this probably needs to be a target whi

[PATCH] D37954: Try to shorten system header paths when using -MD depfiles

joerg added a comment. The behavior of sometimes transforming the path and sometimes not is IMO completely unacceptable. I don't care if GCC created that bug first. Repository: rL LLVM https://reviews.llvm.org/D37954 ___ cfe-commits mailing list

[PATCH] D37954: Try to shorten system header paths when using -MD depfiles

joerg added a comment. This is not about any operating system, but basic consistent behavior. either do the canonicalisation or not. Doing it sometimes is just bogus. You've effectively implemented -fcanonical-system-headers=sometimes. Repository: rL LLVM https://reviews.llvm.org/D37954

[PATCH] D39162: [test] Fix clang-test for FreeBSD and NetBSD

joerg added a comment. I think we should special case Darwin and Windows and fall-back to LD_LIBRARY_PATH for the rest. Can't remember if there is a UNIX-like platform left where it doesn't work. https://reviews.llvm.org/D39162 ___ cfe-commits mai

[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size

joerg added a comment. I've looked at this in some detail now. I'm not exactly sure yet why it is broken. The patch seems quite wrong to me. DW_CFA_GNU_args_size should be applied only when unwinding a call instruction and that regard, the commit message of the original change is quite correct.

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

joerg added a comment. Why again is this a good idea? This is an even worse hack than -Bsymbolic, the latter at least is visible in ELF header without code inspection. This is breaking core premises of ELF. https://reviews.llvm.org/D39079 ___ cfe-

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

joerg added a comment. Let me phrase it differently. What is this patch (and the matching backend PR) supposed to achieve? There are effectively two ways to get rid of PLT entries: (1) Bind references locally. This is effectively what -Bsymbolic does and what is breaking the ELF interposition ru

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

joerg added a comment. In https://reviews.llvm.org/D39079#905396, @rnk wrote: > In https://reviews.llvm.org/D39079#905372, @joerg wrote: > > > Why again is this a good idea? > > > It saves the direct jump to the PLT, reducing icache pressure, which is a > major cost in some workloads. It also

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

joerg added a comment. In https://reviews.llvm.org/D39079#905468, @rnk wrote: > In https://reviews.llvm.org/D39079#905454, @joerg wrote: > > > It also increases the pressure on the branch predictor, so it is not really > > black and white. > > > I don't understand this objection. I'm assuming th

[PATCH] D43871: [modules] No longer include stdlib.h from mm_malloc.h.

joerg added a comment. Please check the history of the file for some of the problems with the redefinition. I'm quite against this change. https://reviews.llvm.org/D43871 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org

[PATCH] D51683: Fix arm_neon.h and arm_fp16.h generation for compiling with std=c89

joerg added inline comments. Comment at: cfe/trunk/utils/TableGen/NeonEmitter.cpp:2412 - OS << "#define __ai static inline __attribute__((__always_inline__, " + OS << "#define __ai static __inline __attribute__((__always_inline__, " "__nodebug__))\n\n"; -

[PATCH] D51683: Fix arm_neon.h and arm_fp16.h generation for compiling with std=c89

joerg added a comment. Correct. The protected name is double underscore as both suffix and prefix. https://reviews.llvm.org/D51683 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49763: [CUDA] Call atexit() for CUDA destructor early on.

joerg added a comment. Can this ever end up in a shared library? If yes, please use the normal logic for creating a global destructor. atexit is not very friendly to dlopen... https://reviews.llvm.org/D49763 ___ cfe-commits mailing list cfe-commits

[PATCH] D49763: [CUDA] Call atexit() for CUDA destructor early on.

joerg added a comment. Depends a bit on the platform, __cxa_atexit on most modern ELF systems, fallback to atexit. If the global dtor is run too late, it smells like a missing library dependency. They are executed in topological order after all. https://reviews.llvm.org/D49763 _

[PATCH] D49771: CodeGen: use non-zero memset when possible for automatic variables

joerg added a comment. There are two different considerations here: (1) Create less target code (2) Create less IR If this code can significantly reduce the amount of IR, it can be useful in general. That's why the existing memset logic is helpful. Repository: rL LLVM https://reviews.llvm.o

[PATCH] D42593: GCC compatibility: Ignore -fstack-clash-protection

joerg added a comment. I really don't like ignoring options that are supposed to provide actual functionality. Most of the other options are for pointless fine tuning and workarounds for broken gcc behavior in ancient versions. Repository: rC Clang https://reviews.llvm.org/D42593 ___

[PATCH] D33726: [driver][netbsd] Build and pass `-L` arguments to the linker

joerg added a comment. I'm against it. I consider this a strong bug on the LLD side and the behavior of clang correct. Repository: rL LLVM https://reviews.llvm.org/D33726 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.

[PATCH] D33726: [driver][netbsd] Build and pass `-L` arguments to the linker

joerg added a comment. Such knowledge is necessary anyway. There is enough software that wants to use the linker directly. Repository: rL LLVM https://reviews.llvm.org/D33726 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.

[PATCH] D33726: [driver][netbsd] Build and pass `-L` arguments to the linker

joerg added a comment. sysroot is already handled in NetBSD.cpp line 118 or so. Repository: rL LLVM https://reviews.llvm.org/D33726 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33726: [driver][netbsd] Build and pass `-L` arguments to the linker

joerg added a comment. A small subset can be found by searching for LINKER_RPATH_FLAG in pkgsrc. A classic offender is Emacs. For more, I would have to systematically search. Repository: rL LLVM https://reviews.llvm.org/D33726 ___ cfe-commits ma

[PATCH] D31972: Do not force the frame pointer by default for ARM EABI

joerg added a comment. Actually, for NetBSD we want to use -fomit-frame-pointer by default whenever the implicit -funwind-tables is also present. In general, that should be the justification for the default behavior: "Can I reliably unwind a binary with the available information?" Performance o

[PATCH] D33726: [driver][netbsd] Build and pass `-L` arguments to the linker

joerg added a comment. In https://reviews.llvm.org/D33726#774105, @ruiu wrote: > I'm totally against adding per-OS path knowledge to our linker. Compilers > already know include paths and I don't want to maintain another list of paths > in the linker. Also this can be more confusing than useful

[PATCH] D34018: Support __float128 on NetBSD libstdc++ x86/x86_64

joerg added a comment. At the very least, missing test case. I'm mostly ambivalent about this -- I don't really see the point and without matching soft float support it won't fully work either. Repository: rL LLVM https://reviews.llvm.org/D34018 __

[PATCH] D34018: Support __float128 on NetBSD libstdc++ x86/x86_64

joerg added a comment. Soft-float on the runtime environment part. I.e. non-trivial operations are lowered to library calls. Repository: rL LLVM https://reviews.llvm.org/D34018 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists

[PATCH] D34018: Support __float128 on NetBSD libstdc++ x86/x86_64

joerg added a comment. As I said, I don't see the point in pretending we support float128 when the runtime doesn't contain the necessary pieces. Repository: rL LLVM https://reviews.llvm.org/D34018 ___ cfe-commits mailing list cfe-commits@lists.l

[PATCH] D34506: Relax an assert in the comparison of source locations

joerg added a comment. I don't think it is a good idea to make this function non-transitive. Repository: rL LLVM https://reviews.llvm.org/D34506 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinf

[PATCH] D41054: Teach clang/NetBSD about additional dependencies for sanitizers

joerg added a comment. I'm not really a fan of linking libutil into all binaries. Why is this code using forkpty in first place and not posix_openpt/grantpt? Repository: rL LLVM https://reviews.llvm.org/D41054 ___ cfe-commits mailing list cfe-co

[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size

joerg updated this revision to Diff 150049. joerg added a comment. After a careful review of newer GCC / libgcc and the assembler annotations from LLVM, I have come to the following conclusions: (1) The semantics have been somewhat changed by GCC in recent years. There is no actual specificatio

[PATCH] D34796: upporting -f(no)-reorder-functions flag, clang side change

joerg added a comment. Excuse me for bring this up so late, but why do we want to make any such promises? As in: fundamentally, LLVM IR doesn't have any order property on the module level. I have yet so seen reasonable code where the order of functions matters for anything but performance. I've

[PATCH] D56215: [lld] [ELF] Include default search paths for NetBSD driver

joerg added inline comments. Comment at: ELF/Driver.cpp:779 +// TODO: verify the triple somehow? +Config->TargetTriple = llvm::Triple(Prefix); + } mgorny wrote: > joerg wrote: > > See ToolChain::getTargetAndModeFromProgramName in clang. > Yes, I've based

[PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

joerg added a comment. That's the other reason why I find the GCC specification as string prefix confusing. I still say we should just go with mapping of path names and then the order question mostly goes away. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D49466

[PATCH] D56215: [lld] [ELF] Include default search paths for NetBSD driver

joerg added a comment. Right, I'm not aware of anyone but FreeBSD really using the OSABI field. For FreeBSD it was a long standing hack around limitations in the ELF kernel loader. I'm not even sure if FreeBSD use(d) to set the OSABI field for the intermediate object files. CHANGES SINCE LAST

[PATCH] D56647: [WIP] [ELF] Implement --copy-dt-needed-entries

joerg added a comment. As first step, it goes into the right direction. I would explicitly set --as-needed for all those indirectly loaded objects. If people want to retain the questionable behavior of newer GNU tools, it could be a separate flag so that a final round can warn if an indirectly

[PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

joerg added a comment. As discussed with dankm on IRC, I still would like to see the correct behavior going into 8.0, i.e. not change it later. Since this also matters for potential faster implementations later, it seems like a good idea to do it now. The changes are well-localized. (1) Do pat

[PATCH] D34018: Support __float128 on NetBSD libstdc++ x86/x86_64

joerg added a comment. Nothing changed. I don't see how catering to the broken libstdc++ self-configuration helps. So no, I still object to this change. Repository: rL LLVM https://reviews.llvm.org/D34018 ___ cfe-commits mailing list cfe-commits

[PATCH] D54657: [clang] Add -MJJ for appending to compilation databases.

joerg added a comment. I don't understand the point here. Why would you want to include pre-processing-only commands in the compilation database? Repository: rC Clang https://reviews.llvm.org/D54657 ___ cfe-commits mailing list cfe-commits@lists

[PATCH] D54657: [clang] Add -MJJ for appending to compilation databases.

joerg added a comment. I'm sorry, but it still sounds to me like you want to address badly written build rules by making the driver more complicated. I don't see that is a reasonable goal forward. Repository: rC Clang https://reviews.llvm.org/D54657 __

[PATCH] D55121: Make several Python scripts portable across Python2 and Python 3

joerg added a comment. Can you split off the pure modernisation changes like new exception or print ? Those are completely non-contentious changes after all. I generally do not like the range and list related changes as many instances are clear regressions for the 2.x case. filter to list compr

[PATCH] D43871: [modules] No longer include stdlib.h from mm_malloc.h.

joerg added a comment. This header is used on systems without glibc. So please don't argue about behavior based only on that. Granted, most other libc implementation are less annoying when it comes to `free` and `malloc`, but still. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D43871/n

[PATCH] D58649: Fix inline assembler constraint validation

joerg created this revision. joerg added a reviewer: compnerd. Herald added a subscriber: eraman. The current constraint logic is both too lax and too strict. It fails for input outside the [INT_MIN..INT_MAX] range, but it also implicitly accepts 0 as value when it should not. Adjust logic to ha

[PATCH] D58649: Fix inline assembler constraint validation

This revision was automatically updated to reflect the committed changes. Closed by commit rL354937: Fix inline assembler constraint validation (authored by joerg, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llv

[PATCH] D58649: Fix inline assembler constraint validation

joerg marked an inline comment as done. joerg added inline comments. Comment at: include/clang/Basic/TargetInfo.h:860 + if (!ImmSet.empty()) +return ImmSet.count(Value.getZExtValue()) != 0; + return !ImmRange.isConstrained || (Value.sge(ImmRange.Min) && Value.s

[PATCH] D58821: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)

joerg added a comment. Can you include a patch for something like (int *)0xdeadbeef on amd64? That's a valid value for "n", but clearly too large for int. Thanks for looking at this, it is one of the two large remaining show stoppers for the asm constraint check. CHANGES SINCE LAST ACTION

[PATCH] D58821: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)

joerg added a comment. The other problem is that we don't use the CFG machinery to prune dead branches. Consider the x86 in/out instructions: one variant takes an immediate, the other a register. The classic way to deal with that is something like static inline void outl(unsigned port, uint32

[PATCH] D58821: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)

joerg added a comment. Well, that was a sample to illustrate the point. A full working (and now failing) example is: static inline void outl(unsigned port, unsigned data) { if (__builtin_constant_p(port) && port < 0x100) { __asm volatile("outl %0,%w1" : : "a"(data), "n"(port)); }

[PATCH] D59118: creduce script for clang crashes

joerg added a comment. For it to be really useful for the majority of bugs, it would be nice to figure out automatically how to get the preprocessing step done and filter out the # lines afterwards. That part alone significantly cuts down the creduce time. CHANGES SINCE LAST ACTION https://r

[PATCH] D60943: Delay diagnosing "n" constraint until after inlining

joerg added a comment. I'm in the process of testing this, but feedback will take a bit. On the more meaty parts of this change, I think further iterations will be necessary in-tree to extend this to the other constraints. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llv

[PATCH] D52137: Added warning for unary minus used with unsigned type

joerg added a comment. I find this warning confusing. I find a4 to be perfectly expected. IMO this warning should be applied only, if the effective value of the expression is not the same as in the modulo-n arithmetic. This means that if (-x) is explicitly or implicitly cast to a less wide unsi

[PATCH] D52696: Update ifunc attribute support documentation

joerg added a comment. I think this is still too optimistic. Full support for ifunc seems to be generally limited to x86. Most other architectures lack even definitions for anonymous ifunc relocations or support proper relaxation only in limited forms. That's especially annoying when looking at

[PATCH] D52696: Update ifunc attribute support documentation

joerg added a comment. Yeah, I would restrict it to just mention that it depends on the target, link time editor and runtime linker. Even the concrete feature set on Linux changes with glibc versions. Repository: rL LLVM https://reviews.llvm.org/D52696 ___

[PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

joerg added inline comments. Comment at: lib/Lex/PPMacroExpansion.cpp:1460 + for (const auto &Entry : MacroPrefixMap) +if (Path.startswith(Entry.first)) + return (Twine(Entry.second) + Path.substr(Entry.first.size())).str(); Lekensteyn wrote: > dankm wr

[PATCH] D58592: [clang] [ToolChains/NetBSD] Support relative libc++ header path

joerg added a comment. I'm not in favor of this. It adds overhead for the system compiler and generally makes the logic more complicated. This seems to be another hack around the fact that the driver has no clear notion of "use system runtime" vs "use custom runtime". Repository: rC Clang

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

joerg added a comment. This version is fine with me. The only contentious part is whether it should be opt-in or opt-out for platforms, so getting this version in and revisiting the issue again later is OK from my perspective. https://reviews.llvm.org/D34158

[PATCH] D37302: [Headers] Define *_HAS_SUBNORM for FLT, DBL, LDBL

joerg added a comment. So what about targets that don't support subnormals? I'm moderately sure ARM falls into this category given the right phase of the moon. https://reviews.llvm.org/D37302 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D37954: Expand absolute system header paths when using -MD depfiles

joerg added a comment. The comments at the very least are misleading. Resolving the path doesn't necessary shorten the string at all, in many cases it will be longer. I'm really not sure if clang should resolve symlinks like this as it can be quite surprising. https://reviews.llvm.org/D37954

[PATCH] D37954: Expand absolute system header paths when using -MD depfiles

joerg added a comment. ninja is not the only consumer of this. For human inspection, it can be quite surprising to see symlinks resolved, i.e. /usr/include/machine on NetBSD. Build systems have different ideas on whether they want absolute resolved paths or not, so it seems like ninja should be

[PATCH] D37954: Try to shorten system header paths when using -MD depfiles

joerg added a comment. Well, the background for the use of the option in NetBSD is related to inducted differences in reproducable builds. From that perspective, it is even worth to sometimes shorten the dependency and sometimes not. https://reviews.llvm.org/D37954 _

[PATCH] D60748: Fix i386 struct and union parameter alignment

joerg added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:1501 +// +// Exclude other System V OS (e.g Darwin, PS4 and FreeBSD) since we don't +// want to spend any effort dealing with the ramifications of ABI breaks. krytarowski wrote: > Dar

[PATCH] D59744: Fix i386 ABI "__m64" type bug

joerg added a comment. I think MMX code is obscure enough at this point that it doesn't matter much either way. Even less across DSO boundaries. That's why I don't really care either way. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59744/new/ https://reviews.llvm.org/D59744 _

[PATCH] D60943: Delay diagnosing asm constraints that require immediates until after inlining

joerg added a comment. You lost the changes to lib/Sema/SemaStmtAsm.cpp to actually do the delaying for immediate operands? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60943/new/ https://reviews.llvm.org/D60943 __

[PATCH] D60943: Delay diagnosing asm constraints that require immediates until after inlining

joerg added a comment. I understand, but the current version just doesn't work anyway to delay it. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60943/new/ https://reviews.llvm.org/D60943 ___ cfe-commits mailing list

[PATCH] D60943: Delay diagnosing asm constraints that require immediates until after inlining

joerg added a comment. The combination of D60942 , D06943 and D65280 solves the problems for me on all targets I have. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60943/new/ https://reviews.ll

[PATCH] D56215: [lld] [ELF] Include default search paths for NetBSD driver

joerg added a comment. This doesn't seem a reasonable approach at all: (1) It breaks cross-linking. (2) It is not correct for any target architecture, e.g. /usr/local/lib certainly doesn't belong on this list and /lib doesn't either. (3) The correct list depends not only on the target architectu

[PATCH] D56215: [lld] [ELF] Include default search paths for NetBSD driver

joerg added a comment. In D56215#1344279 , @krytarowski wrote: > In D56215#1344183 , @joerg wrote: > > > This doesn't seem a reasonable approach at all: > > > > (1) It breaks cross-linking. > > (2) It is not correc

[PATCH] D56215: [lld] [ELF] Include default search paths for NetBSD driver

joerg added a comment. Talking from the perspective of having had to deal with thousands of packages in pkgsrc over the years: it is naive to believe that there isn't a lot of software that calls the linker directly for various reasons. As such, it is very important to have a useful configurati

[PATCH] D56215: [lld] [ELF] Include default search paths for NetBSD driver

joerg added a comment. Thanks, this looks like a good starting point. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56215/new/ https://reviews.llvm.org/D56215 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi

[PATCH] D56215: [lld] [ELF] Include default search paths for NetBSD driver

joerg added inline comments. Comment at: ELF/Driver.cpp:779 +// TODO: verify the triple somehow? +Config->TargetTriple = llvm::Triple(Prefix); + } See ToolChain::getTargetAndModeFromProgramName in clang. CHANGES SINCE LAST ACTION https://reviews.llvm

[PATCH] D56215: [lld] [ELF] Include default search paths for NetBSD driver

joerg added a comment. @ruiu: No, it is exactly what you want, since it allows you to point lld into the normal sysroot. Cross-compiling is the default case for the NetBSD toolchain. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56215/new/ https://reviews.llvm.org/D56215 __

[PATCH] D56215: [lld] [ELF] Include default search paths for NetBSD driver

joerg added inline comments. Comment at: ELF/Driver.cpp:381 +} +Config->SearchPaths.push_back("/usr/lib"); + } Those need to be sysroot-relative of course. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56215/new/ https://reviews.llvm.org/D5621

[PATCH] D67638: Improve __builtin_constant_p lowering

joerg created this revision. joerg added a reviewer: rsmith. Herald added subscribers: kristina, kbarton, nemanjai. Herald added a project: clang. Herald added a subscriber: wuzish. __builtin_constant_p used to be short-cut evaluated to false when building with -O0. This is undesirable as it mean

[PATCH] D67638: Improve __builtin_constant_p lowering

joerg added a comment. Ping. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67638/new/ https://reviews.llvm.org/D67638 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/lis

[PATCH] D41240: [Solaris] __float128 is supported on Solaris/x86

joerg added a comment. Things are different for a libgcc-based toolchain and a compiler-rt based toolchain. Repository: rL LLVM https://reviews.llvm.org/D41240 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin

[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size

joerg added a comment. I'm back to the point where I can't reproduce the problem :( Can we start providing an actual failing test case? It's annoying to debug a problem when you can't reproduce it. https://reviews.llvm.org/D38680 ___ cfe-commits m

[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size

This revision was automatically updated to reflect the committed changes. Closed by commit rL337312: The semantics of DW_CFA_GNU_args_size have changed subtile over the (authored by joerg, committed by ). Herald added subscribers: llvm-commits, christof. Changed prior to commit: https://reviews

[PATCH] D49480: Haiku: support for secondary arch

joerg requested changes to this revision. joerg added a comment. This revision now requires changes to proceed. This is absolutely not how the clang driver is supposed to work. No conditional compilation. Repository: rC Clang https://reviews.llvm.org/D49480 ___

[PATCH] D49482: Haiku: add a test for haiku driver

joerg added a comment. It seems to miss most of the interesting checks, i.e. crt files. Compare with any of the entries on netbsd.c for example. Repository: rC Clang https://reviews.llvm.org/D49482 ___ cfe-commits mailing list cfe-commits@lists.

[PATCH] D49481: Haiku: Enable thread-local storage and disable PIE by default

joerg requested changes to this revision. joerg added a comment. This revision now requires changes to proceed. Both needs a test case :) Repository: rC Clang https://reviews.llvm.org/D49481 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D49466: Initial implementation of -fmacro-prefix-mapand -ffile-prefix-map

joerg added inline comments. Comment at: lib/Driver/ToolChains/Clang.cpp:609 static void addDebugPrefixMapArg(const Driver &D, const ArgList &Args, ArgStringList &CmdArgs) { + for (const Arg *A : Args.filtered(options::OPT_ffile_prefix_map_EQ)) { +StringRef Map = A->getVal

[PATCH] D68410: [AttrDocs] document always_inline

joerg added a comment. I wonder if we should actually enumerate evil here, i.e. give the situations in which inlining actually fails. As mentioned on IRC, I wonder if we shouldn't aim for the stronger semantics and at least warn by default of any situation that prevents always_inline from doing

[PATCH] D67638: Improve __builtin_constant_p lowering

This revision was automatically updated to reflect the committed changes. Closed by commit rG529f4ed401ea: Improve __builtin_constant_p lowering (authored by joerg). Changed prior to commit: https://reviews.llvm.org/D67638?vs=220398&id=224796#toc Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D80300: [Driver] Add DEFAULT_DYLD_PREFIX and DEFAULT_RPATH to complement DEFAULT_SYSROOT

joerg added a comment. I do agree with the feature request, but I'm not sure about the implementation. It doesn't seem to work well with the cross-compiling support in the driver as clearly shown by the amount of tests that need patching. Is cross-compiling a concern for you at all? Otherwise I

[PATCH] D80300: [Driver] Add DEFAULT_DYLD_PREFIX and DEFAULT_RPATH to complement DEFAULT_SYSROOT

joerg added a comment. I see that more as a short-coming in the existing DEFAULT_SYSROOT behavior and less an argument for making more cases like it. So the general idea is that for turnkey toolchains, we want to allow customizing the "default" target of the toolchain to hard-code options like

[PATCH] D80300: [Driver] Add DEFAULT_DYLD_PREFIX and DEFAULT_RPATH to complement DEFAULT_SYSROOT

joerg added a comment. I don't agree with the justification at all, but it also seems that noone else cares about the build option creep here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80300/new/ https://reviews.llvm.org/D80300

[PATCH] D73245: Avoid using std::max_align_t in pre-C++11 mode

joerg added a comment. ping2 Louis, did I answer your questions? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73245/new/ https://reviews.llvm.org/D73245 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bi

[PATCH] D73245: Avoid using std::max_align_t in pre-C++11 mode

This revision was not accepted when it landed; it landed in state "Needs Revision". This revision was automatically updated to reflect the committed changes. Closed by commit rG98f77828a98f: Avoid using std::max_align_t in pre-C++11 mode (authored by joerg). Herald added a project: libc++. Herald

[PATCH] D77697: libc++: adjust modulemap for non-modular C

joerg added a comment. This fixes the module build of clang for me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77697/new/ https://reviews.llvm.org/D77697 ___ cfe-commits mailing list cfe-commits@lis

[PATCH] D77697: libc++: adjust modulemap for non-modular C

joerg added a comment. @ldionne I was updating libc++ from d42baff45d9700a199982ba0ac04dbc6c6d911bb and LLVM itself from 38aebe5c04ab4cb3695dc1bcc60b9a7b55215aff

[PATCH] D122077: [InstCombine] Fold (ctpop(X) == 1) | (X == 0) into ctpop(X) < 2

joerg added a comment. Why is this fold preferable to `(X & (X-1)) == 0`? At least on all architectures without native population count, the binary-and based test is preferable and it might even be better with it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://revie

[PATCH] D106577: [clang] Define __STDC_ISO_10646__

joerg added a comment. This patch is certainly wrong for NetBSD as the wchar_t encoding is up to the specific locale charset and *not* UCS-2 or UCS-4 for certain legacy encodings like the various shift encodings in East Asia. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION h

[PATCH] D106577: [clang] Define __STDC_ISO_10646__

joerg added a comment. In D106577#2899715 , @aaron.ballman wrote: > In D106577#2899711 , @joerg wrote: > >> This patch is certainly wrong for NetBSD as the wchar_t encoding is up to >> the specific locale charse

[PATCH] D106577: [clang] Define __STDC_ISO_10646__

joerg added a comment. > I still don't fully understand the original comment from Joerg. The encoding > of `L'a'` cannot change at runtime; it's a literal whose encoding is decided > entirely at compile time. @joerg -- did you mean that Clang produces a > different literal encoding depending on

[PATCH] D121512: [Support] Change zlib::compress to return void

joerg added a comment. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121512/new/ https://reviews.llvm.org/D121512 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-b

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

joerg added a comment. As is, I think this conflicts with `-ffreestanding` assumptions or at the very least the spirit. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123345/new/ https://reviews.llvm.org/D123345 ___

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

joerg added a comment. The patch contains at least one user visible change that would be quite surprising: it is no longer possible to intentionally set a break point on `std::move`. Thinking more about it, what about a slightly different implementation strategy? Provide a compiler built-in `_

  1   2   3   >