[PATCH] D81422: Change filecheck default to dump input on failure

mehdi_amini added a comment. I'm thinking about a possible improvement here: we could have FileCheck dump the input for the current CHECK-LABEL section only: it seems like it could reduce the output drastically while still preserving how useful the information is? Repository: rG LLVM Github

[PATCH] D81422: Change filecheck default to dump input on failure

mehdi_amini added a comment. In D81422#2090618 , @jdenny wrote: > In D81422#2090425 , @mehdi_amini > wrote: > > > I'm thinking about a possible improvement here: we could have FileCheck > > dump the input for the

[PATCH] D81045: [LLVM] Change isa<> to a variadic function template

mehdi_amini added a comment. Do you need someone to land this for you? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81045/new/ https://reviews.llvm.org/D81045 ___ cfe-commits mailing list cfe-commits@

[PATCH] D81223: Size LTO (1/3): Standardizing the use of OptimizationLevel

mehdi_amini added a comment. In D81223#2087660 , @rcorcs wrote: > The way I see it, with size level for LTO, we could have a different LTO > optimization pipeline for size or runtime performance. So this is the important point to settle before going on

[PATCH] D81422: Change filecheck default to dump input on failure

mehdi_amini added a comment. In D81422#2083761 , @arsenm wrote: > I think this is a worse default for development for large tests. Maybe the issue is with large tests that needs to be broken up? To me this is a general improvement during development at m

[PATCH] D81422: Change filecheck default to dump input on failure

mehdi_amini added a comment. In D81422#2083882 , @arsenm wrote: > In D81422#2083808 , @mehdi_amini > wrote: > > > In D81422#2083761 , @arsenm wrote: > > > > > I think this i

[PATCH] D81422: Change filecheck default to dump input on failure

This revision was automatically updated to reflect the committed changes. Closed by commit rGd31c9e5a46ee: Change filecheck default to dump input on failure (authored by mehdi_amini). Changed prior to commit: https://reviews.llvm.org/D81422?vs=269581&id=269631#toc Repository: rG LLVM Github

[PATCH] D81422: Change filecheck default to dump input on failure

mehdi_amini updated this revision to Diff 269581. mehdi_amini marked 4 inline comments as done. mehdi_amini edited the summary of this revision. mehdi_amini added a comment. Address @jdenny's comments: - fix the example in lit.local.cfg - Test the default value for dump-input Repository: rG L

[PATCH] D81422: Change filecheck default to dump input on failure

mehdi_amini added inline comments. Comment at: llvm/test/FileCheck/lit.local.cfg:42 # ; RUN: %ProtectFileCheckOutput FILECHECK_OPTS=-v \ # ; RUN: FileCheck -input-file %s %s 2>&1 \ # ; RUN: | FileCheck -check-prefix TRACE %s jdenny wrote: > Please add `-

[PATCH] D81422: Change filecheck default to dump input on failure

mehdi_amini added inline comments. Comment at: llvm/test/FileCheck/dump-input-enable.txt:78 ; Check no -dump-input, which defaults to never. ;-- jdenny wrote: > Are the tests in this section passing for you now

[PATCH] D81422: Change filecheck default to dump input on failure

mehdi_amini added a comment. In D81422#2080758 , @probinson wrote: > I don't remember the exact reasoning but I believe it had something to do > with bot logs? @jdenny or @thopre might remember. It'd be interesting to hear about it: having the bot log

[PATCH] D81422: Change filecheck default to dump input on failure

mehdi_amini updated this revision to Diff 269385. mehdi_amini marked 3 inline comments as done. mehdi_amini added a comment. Herald added a subscriber: delcypher. Fix more tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81422/new/ https://revie

[PATCH] D81422: Change filecheck default to dump input on failure

mehdi_amini updated this revision to Diff 269342. mehdi_amini added a comment. Herald added subscribers: Sanitizers, cfe-commits, msifontes, jurahul, Kayjukh, frgossen, grosul1, Joonsoo, stephenneuendorffer, liufengdb, aartbik, lucyrfox, mgester, arpith-jacob, csigg, nicolasvasilache, antiagainst

[PATCH] D81223: Size LTO (1/3): Standardizing the use of OptimizationLevel

mehdi_amini added a comment. In D81223#2076420 , @rcorcs wrote: > This patch standardizes the use of OptimizationLevel across PassBuilder, > PassManagerBuilder, LTO configuration, and LTO code generators. Even with > this patch, further work is still nee

[PATCH] D81223: Size LTO (1/3): Standardizing the use of OptimizationLevel

mehdi_amini added a comment. > This patch is the first in the sequence of three patches for supporting size > optimization with LTO. Can you start by describing the problem you're trying to solve and the overall approach, including the end-to-end user-interface? Repository: rG LLVM Github M

[PATCH] D79919: [Driver] Pass -plugin-opt=O2 for -Os -Oz and -plugin-opt=O1 for -Og

mehdi_amini added a comment. LGTM, probably want @pcc to approve as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79919/new/ https://reviews.llvm.org/D79919 ___ cfe-commits mailing list cfe-commi

[PATCH] D77859: Revert "[DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff."

mehdi_amini added a comment. Was reverted, can we drop this one? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77859/new/ https://reviews.llvm.org/D77859 ___ cfe-commits mailing list cfe-commits@lists.

[PATCH] D77632: [TLI] Per-function fveclib for math library used for vectorization

mehdi_amini added a comment. In D77632#1976231 , @wenlei wrote: > And agree with @tejohnson, if the openness is a feature, it should be covered > in tests, otherwise it can feel somewhat like a loophole and prone to breakage The thing is that LLVM does

[PATCH] D77632: [TLI] Per-function fveclib for math library used for vectorization

mehdi_amini added a comment. In D77632#1976126 , @tejohnson wrote: > I think this should work. Just reiterating something we chatted about off > patch yesterday, we really need a unit test that mimics the behavior utilized > by the XLA compiler, for regr

[PATCH] D77925: Revert "[TLI] Per-function fveclib for math library used for vectorization"

This revision was automatically updated to reflect the committed changes. Closed by commit rGed03d9485eb5: Revert "[TLI] Per-function fveclib for math library used for vectorization" (authored by mehdi_amini). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm

[PATCH] D77632: [TLI] Per-function fveclib for math library used for vectorization

mehdi_amini added a comment. The existing TLI provides a very convenient way to define a VecLib without LLVM knowing about it ahead of time. This feature is important for any embedded use of LLVM as a library out-of-tree (I'll add a unit-test in-tree). I don't think it is a big change to this pa

[PATCH] D77925: Revert "[TLI] Per-function fveclib for math library used for vectorization"

mehdi_amini created this revision. mehdi_amini added a reviewer: tejohnson. Herald added subscribers: cfe-commits, haicheng, hiraditya, eraman. Herald added a project: clang. mehdi_amini added a reviewer: wenlei. tejohnson accepted this revision. tejohnson added a comment. This revision is now acce

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

mehdi_amini added inline comments. Comment at: llvm/include/llvm/Support/GenericDomTree.h:32 #include "llvm/ADT/SmallVector.h" +#include "llvm/IR/CFGDiff.h" #include "llvm/Support/CFGUpdate.h" mehdi_amini wrote: > dblaikie wrote: > > mehdi_amini wrote: > > > Th

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

mehdi_amini added inline comments. Comment at: llvm/include/llvm/Support/GenericDomTree.h:32 #include "llvm/ADT/SmallVector.h" +#include "llvm/IR/CFGDiff.h" #include "llvm/Support/CFGUpdate.h" dblaikie wrote: > mehdi_amini wrote: > > This looks like a layering

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

mehdi_amini added inline comments. Comment at: llvm/include/llvm/Support/GenericDomTree.h:32 #include "llvm/ADT/SmallVector.h" +#include "llvm/IR/CFGDiff.h" #include "llvm/Support/CFGUpdate.h" This looks like a layering violation to me here: I wouldn't expect a

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

mehdi_amini added a comment. In D77341#1973827 , @bondhugula wrote: > @asbirlea This has broken the MLIR build. Could you please build with > `-DLLVM_ENABLE_PROJECTS=mlir`? If you use arcanist, you get pre-merge testing on Phabricator :) Repository:

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

mehdi_amini added a comment. But this sentence alone at the end of this paragraph does not solve anything to me: how is one know that someone in an "area" (whatever that means) is expected to review all the "non-trivial" patches pre-commit? I don't necessarily disagree with your underlying inte

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

mehdi_amini added a comment. I am still not sure what "if someone has asked for extra review of a specific area" refers to? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77683/new/ https://reviews.llvm.org/D77683 ___

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

mehdi_amini added a comment. This seems like both over-fitting the general rule for a specific case and fuzzy/imprecise enough that it isn't clear to me how much useful it is in practice? Maybe we could step back: what is the concrete problem that this intends to address? Is there an area wher

[PATCH] D75153: [ThinLTO] Allow usage of all SMT threads in the system

mehdi_amini added a comment. > /opt:lldltojobs=N -- limit usage to N threads, but constrained by usage of > heavyweight_hardware_concurrency(). I really dislike this behavior: this seems user hostile to me. I would either: - honor the user request (eventually display a warning), this is in line

[PATCH] D71775: [ThreadPool] On Windows, extend usage to all CPU sockets and all NUMA groups

mehdi_amini added inline comments. Comment at: llvm/lib/Support/Threading.cpp:94 + // uselessly induce more context-switching and cache eviction. + if (!ThreadsRequested || ThreadsRequested > (unsigned)MaxThreadCount) +return MaxThreadCount; aganea wrote: >

[PATCH] D71775: [ThreadPool] On Windows, extend usage to all CPU sockets and all NUMA groups

mehdi_amini added a comment. There is something puzzling to me in this patch in the way the ThreadPool was modified: the client of the thread pool cannot request a number of threads anymore, but only a *maximum* number of threads. I don't really understand why? Using the OS reported concurrency

[PATCH] D74878: [remark][diagnostics] [codegen] Fix PR44896

mehdi_amini added a comment. > It seems to me that we are not dropping the flag of -fdiscard-value-names. I also reads this as overriding a cc1 flag / ignoring the flag, I don't know if we consistently warn in such cases though. Overall LGTM for the fix, pending resolution for the warning, than

[PATCH] D74871: Fix interaction between -fdiscard-value-names and LLVM Bitcode

mehdi_amini added a comment. You mention a test case in the description but I don't see it? Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3607 +// Cannot discard value names when processing llvm-ir, because IR loading +// is conservative wrt. names. +if (Res

[PATCH] D71687: Fix full loop unrolling initialization in new pass manager

mehdi_amini added inline comments. Comment at: llvm/lib/Passes/PassBuilder.cpp:484 +LPM2.addPass(LoopFullUnrollPass(Level, +/*OnlyWhenForced=*/!PTO.LoopUnrolling, PTO.ForgetAllSCEVInLoopUnroll)); ---

[PATCH] D71775: [ThreadPool] On Windows, extend usage to all CPU sockets and all NUMA groups

mehdi_amini added a comment. > Will it make sense to say "I don't want hyper-threads" ? Not sure I remember correctly, but I believe one motivation behind avoiding "hyper-threads" and other virtual cores was that while they improve slightly the performance, they also increase the peak memory re

[PATCH] D71775: [ThreadPool] On Windows, extend usage to all CPU sockets and all NUMA groups

mehdi_amini added a comment. Also: using heavyweight_hardware_concurrency() in the linker but having multiple linker jobs schedules by the build system was another reason (I think LLVM CMake default to 2 parallel link jobs when using ThinLTO for instance). Repository: rG LLVM Github Monorepo

[PATCH] D69498: IR: Invert convergent attribute handling

mehdi_amini added a comment. Talked with Matt at the last social, they thought I was strongly opposed to this: I am not, and I told them I would clarify here: I don't buy the argument of "frontend projects have consistently run into problems related to this", I think this is not a good justific

[PATCH] D65410: [PassManager] First Pass implementation at -O1 pass pipeline

mehdi_amini added a comment. LGTM to land this and iterate, but you likely want someone else to confirm :) Comment at: llvm/test/Other/new-pm-defaults.ll:253 +; CHECK-Os-NEXT: Finished llvm::Function pass manager run. +; CHECK-Oz-NEXT: Finished llvm::Function pass manager run.

[PATCH] D69498: IR: Invert convergent attribute handling

mehdi_amini added a comment. In D69498#1735763 , @arsenm wrote: > In D69498#1731265 , @mehdi_amini > wrote: > > > Just thought about a slight variation on this: what about adding a flag on > > the datalayout (or a

[PATCH] D69498: IR: Invert convergent attribute handling

mehdi_amini added a comment. In D69498#1727650 , @dexonsmith wrote: > In D69498#1723606 , @rjmccall wrote: > > > Perhaps there should be a global metadata, or something in the > > increasingly-misnamed "data layout

[PATCH] D69498: IR: Invert convergent attribute handling

mehdi_amini added a comment. In D69498#1727546 , @jdoerfert wrote: > In D69498#1727419 , @mehdi_amini > wrote: > > > In D69498#1727080 , @jdoerfert > > wrote: > > > > > Let

[PATCH] D69498: IR: Invert convergent attribute handling

mehdi_amini added a comment. In D69498#1727080 , @jdoerfert wrote: > Let me quote @arsenm here because this is so important: "Basically no > frontend has gotten this right, including clang and non-clang frontends." For > me, that statement alone is reaso

[PATCH] D69498: IR: Invert convergent attribute handling

mehdi_amini added a comment. In D69498#1725867 , @jdoerfert wrote: > In D69498#1725819 , @mehdi_amini > wrote: > > > Maybe we can start by looking into the motivation for this patch: > > > > > There is a burden on

[PATCH] D69498: IR: Invert convergent attribute handling

mehdi_amini added a comment. Maybe we can start by looking into the motivation for this patch: > There is a burden on frontends in environments that care about convergent > operations to add the attribute just in case it is needed. This has proven to > be somewhat problematic, and different fro

[PATCH] D64742: Allow using -ftrivial-auto-var-init=zero in C mode without extra flags

mehdi_amini added a comment. In D64742#1606244 , @glider wrote: > As a data point, Linus Torvalds suggested that we need a similar feature for > GCC so that the "kernel C standard" mandates zero-initialization for locals: > https://lkml.org/lkml/2019/7/2

[PATCH] D68049: Propeller: Clang options for basic block sections

mehdi_amini added inline comments. Comment at: clang/include/clang/Basic/CodeGenOptions.def:52 + ///< Produce unique section names with + ///< basic block sections. ENUM_CODEGENOPT(FramePointer, FramePoin

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

mehdi_amini added a comment. In D63932#1679910 , @ostannard wrote: > > This makes the IR self-contained, which is good, but it also make the > > interpretation of the IR modal, which isn't great. It means that suddenly > > the rules of interpretation of

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

mehdi_amini added a comment. > we represent the "post-link" flag in the IR (e.g. as a module flag) in order > to keep the IR self-contained. This makes the IR self-contained, which is good, but it also make the interpretation of the IR modal, which isn't great. It means that suddenly the rules

[PATCH] D63976: Allow clang -Os and -Oz to work with -flto and lld

mehdi_amini added a comment. > with a funny command line like `clang -Oz -flto -o foo a.o b.c`, where one > (or all) of the input arguments is a source file? Would it invoke the bitcode > compile of the source files with the requested `-Oz` and then invoke the LTO > linker plugin with `-O2`/`-O

[PATCH] D63976: Allow clang -Os and -Oz to work with -flto and lld

mehdi_amini added a comment. > I assume I might be missing something here, though, since someone mentioned > this above (I don't understand the response to it though). There are two invocations in LTO: the first phase where we compile from source to bitcode, and the second phase which is invoke

[PATCH] D63976: Allow clang -Os and -Oz to work with -flto and lld

mehdi_amini added a comment. In D63976#1566236 , @ruiu wrote: > I agree with Teresa. I don't think automatically setting O3 > for Os and Oz is a good idea > because these three options are different (that's wh

[PATCH] D61022: [ThinLTO] Pass down opt level to LTO backend and handle -O0 LTO in new PM

mehdi_amini added inline comments. Comment at: llvm/test/tools/gold/X86/opt-level.ll:53 + ; CHECK-O1-OLDPM: select + ; The new PM does not do as many optimizations at O1 + ; CHECK-O1-NEWPM: phi tejohnson wrote: > chandlerc wrote: > > tejohnson wrote: > > > tej

[PATCH] D61022: [ThinLTO] Pass down opt level to LTO backend and handle -O0 LTO in new PM

mehdi_amini added inline comments. Comment at: llvm/test/tools/gold/X86/opt-level.ll:53 + ; CHECK-O1-OLDPM: select + ; The new PM does not do as many optimizations at O1 + ; CHECK-O1-NEWPM: phi This is intended? I'm surprised the two PMs don't have the same li

[PATCH] D31739: Add markup for libc++ dylib availability

mehdi_amini marked an inline comment as done. mehdi_amini added subscribers: ldionne, Bigcheese. mehdi_amini added inline comments. Comment at: libcxx/trunk/test/std/thread/thread.mutex/thread.lock/thread.lock.shared/lit.local.cfg:1 +if 'availability' in config.available_feature

[PATCH] D58157: Stop enabling clang-tools-extra automatically when clang is in LLVM_ENABLE_PROJECTS

mehdi_amini added a comment. In D58157#1397302 , @rnk wrote: > In D58157#1397274 , @mehdi_amini > wrote: > > > It'd still be nice to identify the end goal with most cfe people (i.e. even > > if you land this right

[PATCH] D58157: Stop enabling clang-tools-extra automatically when clang is in LLVM_ENABLE_PROJECTS

mehdi_amini added a comment. In D58157#1396824 , @rnk wrote: > And, this change really just keeps us at parity with what we had with svn. > We can always revisit the decision to merge the clang tools into clang. This > particular change just gives us m

[PATCH] D58157: Stop enabling clang-tools-extra automatically when clang is in LLVM_ENABLE_PROJECTS

mehdi_amini added a comment. In D58157#1396072 , @thakis wrote: > In D58157#1395762 , @mehdi_amini > wrote: > > > In D58157#1395716 , @rnk wrote: > > > > > I think we have c

[PATCH] D57330: Adjust documentation for git migration.

mehdi_amini added a comment. > You can avoid the git status pollution by adding the build directory to > .git/info/exclude. Good to know! Should we include this in the doc? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57330/new/ https://reviews.llvm.org/D57330

[PATCH] D57330: Adjust documentation for git migration.

mehdi_amini added a comment. In D57330#1375096 , @labath wrote: > This is not an full out-of-source build, since the build folder is still a > subfolder of the repo root My definition of what qualify an "out-of-source" build is that the build process w

[PATCH] D57330: Adjust documentation for git migration.

mehdi_amini added a comment. LGTM, but for one comment that requires a fix I believe (lld on Windows) Comment at: libcxx/docs/BuildingLibcxx.rst:47 - * ``cd build`` - * ``cmake -G [options] `` So nice to see these steps going away :) C

[PATCH] D51899: Remove extraneous ".a" suffix from baremetal clang_rt.builtins when compiling for baremetal.

mehdi_amini added a comment. @kristina both bfd and lld are adding the `.a ` extension when you pass a lib through -l. Similarly to -llibfoo won't work, -lfoo.a will lead the linker to look for libfoo.a.a https://reviews.llvm.org/D51899 ___ cfe-c

[PATCH] D50882: [ThinLTO] Correct documentation on default number of threads

mehdi_amini accepted this revision. mehdi_amini added a comment. This revision is now accepted and ready to land. LGTM Repository: rC Clang https://reviews.llvm.org/D50882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.

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

mehdi_amini added a comment. > I'm worried, however, about generating a bunch more code than needed from > clang in the hopes that the compiler will clean it up later. Isn't a strong design component of clang/LLVM? Clang does not try to generate "smart" code and leave it up to LLVM to clean it

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

mehdi_amini added a comment. In https://reviews.llvm.org/D49771#1183380, @jfb wrote: > In https://reviews.llvm.org/D49771#1181008, @mehdi_amini wrote: > > > I'm curious: isn't the kind of optimization we should expect LLVM to > > provide? > > > Maybe? It seems obvious to do here since we know we

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

mehdi_amini added a comment. I'm curious: isn't the kind of optimization we should expect LLVM to provide? Repository: rL LLVM https://reviews.llvm.org/D49771 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/m

[PATCH] D34992: Emit static constexpr member as available_externally definition

mehdi_amini added a comment. > I'd like to also see a testcase for the situation where we trigger the > emission of a declaration with an available_externally definition and then > find we need to promote it to a "full" definition: Added! Comment at: clang/lib/CodeGen/CodeGe

[PATCH] D34992: Emit static constexpr member as available_externally definition

This revision was automatically updated to reflect the committed changes. Closed by commit rL311857: Emit static constexpr member as available_externally definition (authored by mehdi_amini). Changed prior to commit: https://reviews.llvm.org/D34992?vs=109694&id=112836#toc Repository: rL LLVM

[PATCH] D34992: Emit static constexpr member as available_externally definition

mehdi_amini marked an inline comment as done. mehdi_amini added a comment. Bi-weekly ping! (@rsmith) https://reviews.llvm.org/D34992 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36713: [libc++] Add a persistent way to disable availability

mehdi_amini added a comment. Seems fine to me. Adding @dexonsmith (I don't know who maintain libc++ at Apple right now) https://reviews.llvm.org/D36713 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/li

[PATCH] D34992: Emit static constexpr member as available_externally definition

mehdi_amini marked 6 inline comments as done. mehdi_amini added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2293 /// If ExcludeCtor is true, the duration when the object's constructor runs -/// will not be considered. The caller will need to verify that the

[PATCH] D34992: Emit static constexpr member as available_externally definition

mehdi_amini updated this revision to Diff 109694. mehdi_amini marked 12 inline comments as done. mehdi_amini added a comment. Address Richard's comment https://reviews.llvm.org/D34992 Files: clang/lib/CodeGen/CodeGenModule.cpp clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp Index: clang/

[PATCH] D34992: Emit static constexpr member as available_externally definition

mehdi_amini added a comment. Ping again @rsmith (or anyone else) https://reviews.llvm.org/D34992 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33900: Print registered targets in clang's version information

mehdi_amini added a comment. In https://reviews.llvm.org/D33900#820281, @hans wrote: > In https://reviews.llvm.org/D33900#818968, @mehdi_amini wrote: > > > I think @thakis is right: this too verbose to be the default --version. > > We likely shouldn't ship this in clang-5.0 (@hans). > > > Let me

[PATCH] D34992: Emit static constexpr member as available_externally definition

mehdi_amini added a comment. Weekly ping! (@rsmith) https://reviews.llvm.org/D34992 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33900: Print registered targets in clang's version information

mehdi_amini added a comment. Here is the current output: clang version 5.0.0 Target: x86_64-apple-darwin16.6.0 Thread model: posix InstalledDir: /Users/mamini/projects/llvm/./clangDebug/bin Registered Targets: aarch64- AArch64 (little endian) aarch64_be - AArch64 (

[PATCH] D34992: Emit static constexpr member as available_externally definition

mehdi_amini added a comment. @rsmith: post-C++-commitee-meeting ping ;) https://reviews.llvm.org/D34992 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35081: [ThinLTO] Allow multiple summary entries.

mehdi_amini added a comment. In https://reviews.llvm.org/D35081#808207, @tejohnson wrote: > My first option was if any copy is under the threshold, simply pick the > prevailing copy (it may be over threshold, but assume we want that one > anyway). Another possibility is to only allow importing

[PATCH] D35081: [ThinLTO] Allow multiple summary entries.

mehdi_amini added a comment. In https://reviews.llvm.org/D35081#808189, @tejohnson wrote: > > Alternatively (and likely preferably from a compile-time point of view to > > limit the list of import), we could keep a map of GUID->Summary and reuse > > it before trying to select a new callee. > >

[PATCH] D35081: [ThinLTO] Allow multiple summary entries.

mehdi_amini added a comment. Thanks, very clear :) I would expect that if we reprocess a GUID we invalidate the previous import of the same GUID. Right now my impression is that the issue is that ` ImportList[ExportModulePath][VI.getGUID()]` is indexed on the importing module. So it'd require

[PATCH] D35081: [ThinLTO] Allow multiple summary entries.

mehdi_amini added a comment. Teresa: can you describe a bit more how we end up importing two summaries for the same GUID? I would expect that after importing one, we don't even come to try to import another since we already have one. https://reviews.llvm.org/D35081 _

[PATCH] D35081: [ThinLTO] Allow multiple summary entries.

mehdi_amini added a comment. In https://reviews.llvm.org/D35081#807172, @fhahn wrote: > It's @yunlian, so if the issue you described above covers his failure, than > that's great. @tejohnson do you have time to work on a fix soon? Otherwise I > could give it a try, I would probably need a few p

[PATCH] D35159: [libcxxabi][demangler] Use an AST to represent the demangled name

mehdi_amini added a comment. In https://reviews.llvm.org/D35159#807075, @dexonsmith wrote: > This LGTM. Mehdi, do you have any other concerns? No other concern (haven't looked further for any either) https://reviews.llvm.org/D35159 ___ cfe-commi

[PATCH] D35159: [libcxxabi][demangler] Use an AST to represent the demangled name

mehdi_amini added inline comments. Comment at: src/cxa_demangle.cpp:44 +class string_ref +{ dexonsmith wrote: > erik.pilkington wrote: > > mehdi_amini wrote: > > > If this is supposed to be *the* ultimate LLVM demangler, can we follow > > > LLVM coding standar

[PATCH] D33842: CodeGen: Fix address space of global variable

mehdi_amini added inline comments. Comment at: cfe/trunk/lib/CodeGen/CodeGenModule.cpp:3759 + unsigned AddrSpace = + VD ? GetGlobalVarAddressSpace(VD) : MaterializedType.getAddressSpace(); + auto TargetAS = getContext().getTargetAddressSpace(AddrSpace); Co

[PATCH] D34992: Emit static constexpr member as available_externally definition

mehdi_amini updated this revision to Diff 105787. mehdi_amini added a comment. Fix issues around mutable fields and regression on "internal", add more testing. https://reviews.llvm.org/D34992 Files: clang/lib/CodeGen/CodeGenModule.cpp clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp Index:

[PATCH] D35159: [libcxxabi][demangler] Use an AST to represent the demangled name

mehdi_amini added inline comments. Comment at: src/cxa_demangle.cpp:87 + +class stream +{ Doc (same for non trivial APIs) Comment at: src/cxa_demangle.cpp:125 + +typedef unsigned stream_position; + Doc Co

[PATCH] D35159: [libcxxabi][demangler] Use an AST to represent the demangled name

mehdi_amini added inline comments. Comment at: src/cxa_demangle.cpp:44 +class string_ref +{ If this is supposed to be *the* ultimate LLVM demangler, can we follow LLVM coding standard? https://reviews.llvm.org/D35159 __

[PATCH] D34992: Emit static constexpr member as available_externally definition

mehdi_amini added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2374 + llvm::Constant *Init = nullptr; + if (D && D->isConstexpr() && !D->isInline() && !D->hasAttr()) { +const VarDecl *InitDecl; rsmith wrote: > Applying this for only `co

[PATCH] D34992: Emit static constexpr member as available_externally definition

mehdi_amini added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2376 +const VarDecl *InitDecl; +const Expr *InitExpr = D->getAnyInitializer(InitDecl); +if (InitExpr) { ahatanak wrote: > Does getAnyInitializer ever return a null poin

[PATCH] D33109: Enhance synchscope representation (clang)

mehdi_amini accepted this revision. mehdi_amini added a comment. Make sure your commit message correctly reference the LLVM change. Comment at: test/CodeGen/ms-barriers-intrinsics.c:39 #endif - Don't remove: file should end with a new line I believe. https:/

[PATCH] D34992: Emit static constexpr member as available_externally definition

mehdi_amini added inline comments. Comment at: clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp:12 +// CHECK: @_ZN3Foo4BAR2E = external constant i32, + static const int BAR2 = 43; +}; Note: I'm not sure if the standard allows us to assume that we can fold the

[PATCH] D34992: Emit static constexpr member as available_externally definition

mehdi_amini created this revision. This enables better optimization, I don't if it is legal c++11 though. https://reviews.llvm.org/D34992 Files: clang/lib/CodeGen/CodeGenModule.cpp clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp Index: clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp ===

[PATCH] D24644: Pass -ffunction-sections/-fdata-sections along to gold-plugin

mehdi_amini added a comment. In https://reviews.llvm.org/D24644#792168, @fhahn wrote: > I'd like to fix PR22999 and was wondering if you think adding a > function-section attribute to the IR would be a viable solution? > > When doing LTO, we could add the same function-section to each function i

[PATCH] D34546: docs: Add documentation for the ThinLTO cache pruning policy string.

mehdi_amini added inline comments. Comment at: clang/docs/ThinLTO.rst:160 + value of 0 forces the scan to occur. The default is every 20 minutes. + Clang Bootstrap pcc wrote: > mehdi_amini wrote: > > Why do we document linker-specific option in clang docs? Is i

[PATCH] D34546: docs: Add documentation for the ThinLTO cache pruning policy string.

mehdi_amini added inline comments. Comment at: clang/docs/ThinLTO.rst:160 + value of 0 forces the scan to occur. The default is every 20 minutes. + Clang Bootstrap Why do we document linker-specific option in clang docs? Is it usual? https://reviews.llvm.org/

[PATCH] D34268: [clang] Fix format specifiers fixits for nested macros

mehdi_amini added a comment. Thanks for fixing! (I'm still not the best qualified to review this) Comment at: lib/Edit/EditedSource.cpp:78 if (I != ExpansionToArgMap.end() && -std::find_if( -I->second.begin(), I->second.end(), [&](const MacroArgUse &U)

[PATCH] D34156: [LTO] Add -femit-summary-index flag to emit summary for regular LTO

mehdi_amini added a comment. OK! Thanks both :) https://reviews.llvm.org/D34156 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34156: [LTO] Add -femit-summary-index flag to emit summary for regular LTO

mehdi_amini added a comment. In https://reviews.llvm.org/D34156#780570, @tobiasvk wrote: > - Change patch to always emit a module summary for regular LTO > > I don't see any real downside of having a summary given the marginal time > and space overhead it takes to construct and save it. I th

[PATCH] D33976: [clang] Fix format specifiers fixits

mehdi_amini added a comment. In https://reviews.llvm.org/D33976#776486, @mehdi_amini wrote: > In https://reviews.llvm.org/D33976#775918, @alexshap wrote: > > > @mehdi_amini , thanks, i see, regarding the "opposite issue" - probably an > > example / test case would be helpful, that looks like a s

<    1   2   3   4   >