[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] 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] 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] 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 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] D33525: [ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM.

mehdi_amini added inline comments. Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:423 + +class AARGetter { + FunctionAnalysisManager &AM; Can't you do it with a lambda? https://reviews.llvm.org/D33525 __

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

This revision was automatically updated to reflect the committed changes. Closed by commit rL304127: IRGen: Add optnone attribute on function during O0 (authored by mehdi_amini). Changed prior to commit: https://reviews.llvm.org/D28404?vs=83480&id=100580#toc Repository: rL LLVM https://revi

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

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

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

mehdi_amini added a comment. Interestingly I got the opposite issue recently: calling through a macro with a single format specifier was *adding* new specific in the fixit in some conditions. I'm not the best person to review this change though, let me add a few folks. Repository: rL LLVM

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

mehdi_amini added a comment. 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 separate issue. > Thanks for adding @ahatanak and @arphaman, that would

[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

[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] 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] 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] 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] 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] 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] 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. > 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] 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] 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] 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] 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] 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#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] 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 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] 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. > 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. 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] 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] 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] 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] 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] 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] D79219: [CMake] Simplify CMake handling for zlib

mehdi_amini added inline comments. Comment at: mlir/examples/standalone/CMakeLists.txt:35 +endif() + include(TableGen) mehdi_amini wrote: > I am a bit unsure that it is desirable that it is desirable to need this > change? > This requires every single user of L

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

mehdi_amini added inline comments. Comment at: mlir/examples/standalone/CMakeLists.txt:35 +endif() + include(TableGen) I am a bit unsure that it is desirable that it is desirable to need this change? This requires every single user of LLVM to do this. Is there a

[PATCH] D84691: [CMake] Move find_package(ZLIB) to LLVMConfig

mehdi_amini added a comment. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84691/new/ https://reviews.llvm.org/D84691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.

[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] 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. 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] 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] 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" mehdi_amini wrote: > dblaikie wrote: > > mehdi_amini wrote: > > > Th

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

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. 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] 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] 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] 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] 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] 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] 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 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 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

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

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG72d20b9604f6: [LLVM] Change isa<> to a variadic function template (authored by jurahul, committed by mehdi_amini). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.o

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

mehdi_amini added a comment. In D81422#2093360 , @arsenm wrote: > The amount of context printed previously was good enough for development use. > If I ever can't figure it out from the diff, I want to look at the output > completely separate from the ter

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

mehdi_amini marked an inline comment as done. mehdi_amini added a comment. In D81422#2096643 , @arsenm wrote: > I would also expect a simple command line flag to llvm-lit to be able to > control this, rather than having to set an environment variable Wo

[PATCH] D65761: Add Windows Control Flow Guard checks (/guard:cf).

mehdi_amini added a comment. It seems like this pass was added in lib/Transforms but does not have any unit-tests (lit tests) provided. It isn't even linked into `opt`. As it is an LLVM IR pass it should be tested as such I believe. Can you provide tests for this? Repository: rG LLVM Github

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

mehdi_amini added a comment. I rather have a non closed list of veclib: if you just have a map keyed by string instead of an enum it should just work out? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77925/new/ https://reviews.llvm.org/D77925 __

[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] D124750: [MLIR] Add a utility to sort the operands of commutative ops

mehdi_amini added a comment. For this to be a usable canonicalization, it is really the case where the operands are already sorted (no-op) that needs to be heavily optimized (that is no complex data structure to populate, etc.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D130689: [LLVM] Update C++ standard to 17

mehdi_amini added a comment. Herald added a subscriber: JDevlieghere. Nice, LGTM, thanks for driving this! > Remember that if we want to adopt some new feature in a bigger way it should > be discussed and added to the CodingStandard document. What does it mean exactly? We can't use **anything**

[PATCH] D130689: [LLVM] Update C++ standard to 17

mehdi_amini added a comment. In D130689#3684333 , @thieta wrote: > In D130689#3684330 , @mehdi_amini > wrote: > >> What does it mean exactly? We can't use **anything** C++17 without writing >> it in the coding s

[PATCH] D130689: [LLVM] Update C++ standard to 17

mehdi_amini added a comment. In D130689#3686760 , @h-vetinari wrote: > My point boils down to: "written using standard C++17 > code" does not sound at all like "core language, no stdlib", but very much > like "core+stdlib". We're allowing C++17 library

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

mehdi_amini added a comment. > . I haven't thought too hard about the performance of that while loop but it > seems good enough to land for now. What's the finality of it? That is: outside of canonicalization what is its purpose? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTIO

[PATCH] D130689: [LLVM] Update C++ standard to 17

mehdi_amini added a comment. > Agreed, I think we need to update the protocol for changing the C++ standard > in the future to account for more testing beforehand. The way I would do it is: wait for a Sunday so that the bots aren't loaded, land the change, wait for a couple of hours to ensure t

[PATCH] D130689: [LLVM] Update C++ standard to 17

mehdi_amini added a comment. > There should be a better way than this. Comprehensive pre-merge testing of > all PRs etc. We already have pre-commit tests on Phabricator on Windows and Linux, but that's not exhaustive and for many reasons I don't believe this is realistic in any way: we will al

[PATCH] D105959: Use ManagedStatic and lazy initialization of cl::opt in libSupport to make it free of global initializer

mehdi_amini added inline comments. Comment at: llvm/lib/Support/CMakeLists.txt:7 +# ManagedStatic can be used to enable lazy-initialization of globals. +add_flag_if_supported("-Werror=global-constructors" WERROR_GLOBAL_CONSTRUCTOR) + MaskRay wrote: > Perhaps move

[PATCH] D105959: Use ManagedStatic and lazy initialization of cl::opt in libSupport to make it free of global initializer

mehdi_amini updated this revision to Diff 359191. mehdi_amini marked 7 inline comments as done. mehdi_amini added a comment. Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman. Herald added projects: clang, clang-tools-extra. Address comment and fix build errors Repository: r

[PATCH] D105959: Use ManagedStatic and lazy initialization of cl::opt in libSupport to make it free of global initializer

mehdi_amini updated this revision to Diff 359196. mehdi_amini added a comment. Fix windows build Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105959/new/ https://reviews.llvm.org/D105959 Files: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp

[PATCH] D105959: Use ManagedStatic and lazy initialization of cl::opt in libSupport to make it free of global initializer

mehdi_amini added a comment. In D105959#2882099 , @bondhugula wrote: > This is a really welcome change! Multiple registration issues were really an > inconvenience - I had no clue this was the pattern to use to fix it. Thanks! To be fair: we can do it

[PATCH] D105959: Use ManagedStatic and lazy initialization of cl::opt in libSupport to make it free of global initializer

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG42f588f39c5c: Use ManagedStatic and lazy initialization of cl::opt in libSupport to make it… (authored by mehdi_amini). Changed prior to commit: h

[PATCH] D105959: Use ManagedStatic and lazy initialization of cl::opt in libSupport to make it free of global initializer

mehdi_amini added a comment. That's interesting! I'm not sure how to get there though: a complete solution should be able to "degrade" to global constructor when the platform-specific logic isn't available (unless we're confident that no such environment can exist?). Repository: rG LLVM Git

[PATCH] D105959: Use ManagedStatic and lazy initialization of cl::opt in libSupport to make it free of global initializer

mehdi_amini added a comment. Something else I'm not sure about is how it works across DSOs: when each LLVM library is linked into its own shared library, is the dynamic linker creating this array when loading the libraries? (I'm starting to doubt about how this would even work on windows) Repo

[PATCH] D121076: [MLIR][Python] Add SCFIfOp Python binding

mehdi_amini added a comment. Can you rebase? Your patch does not apply apparently Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121076/new/ https://reviews.llvm.org/D121076 ___ cfe-commits mailing list c

[PATCH] D121076: [MLIR][Python] Add SCFIfOp Python binding

mehdi_amini added a comment. Seems like there is a local base commit in your repository: you're not uploading a diff that applies on HEAD. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121076/new/ https://reviews.llvm.org/D121076

[PATCH] D121076: [MLIR][Python] Add SCFIfOp Python binding

This revision was automatically updated to reflect the committed changes. Closed by commit rG036088fd6ea2: [MLIR][Python] Add SCFIfOp Python binding (authored by chhzh123, committed by mehdi_amini). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D12107

[PATCH] D121549: Define ABI breaking class members correctly

mehdi_amini added inline comments. Comment at: mlir/lib/Target/SPIRV/Deserialization/Deserializer.h:601 + // TODO: place logger under #if LLVM_ENABLE_ABI_BREAKING_CHECKS #ifndef NDEBUG This is a private header (you're in the `lib` directory and not in the `i

[PATCH] D121549: Define ABI breaking class members correctly

mehdi_amini accepted this revision. mehdi_amini added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121549/new/ https://reviews.llvm.org/D121549 ___ cfe-commits mailing list cfe

[PATCH] D123297: [flang][driver] Add support for -mmlir

mehdi_amini added a comment. `-mmlir` is fine with me, but note that MLIR has much less global options than LLVM: you will only get access to context and passmanager options and not individual passes flags. That's not a criticism :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACT

[PATCH] D119136: [clang] Implement Change scope of lambda trailing-return-type

mehdi_amini added a comment. Seems like this is breaking clang bootstrapping? llvm/include/llvm/CodeGen/LiveInterval.h:630:53: error: captured variable 'Idx' cannot appear here [=](std::remove_reference_t V, ^ Repository:

[PATCH] D119136: [clang] Implement Change scope of lambda trailing-return-type

mehdi_amini added a comment. In D119136#3449325 , @cor3ntin wrote: > Yes, working on a fix as we speak. > The meaning of that code changed (in all c++ language modes), I'm > currently trying to find if we have any other issue of that sort and will > commi

[PATCH] D72404: [ThinLTO/FullLTO] Support Os and Oz

mehdi_amini added a comment. In D72404#3304205 , @aykevl wrote: > I would be very interested in this patch, because for me to use ThinLTO > without size regressions I need to set the optimization size level in the > linker (`PassManagerBuilder.SizeLevel`

[PATCH] D119257: Revert "Re-land [LLD] Remove global state in lldCommon"

mehdi_amini added a comment. (The discussion seems to be happening in two places: https://github.com/llvm/llvm-project/issues/53475 ) You can break downstream users, the relationship inside the monorepo is different as far as I know: I believe we will have to live with revert like the one abov

[PATCH] D119257: Revert "Re-land [LLD] Remove global state in lldCommon"

mehdi_amini added a comment. Yes, I saw these, thanks a lot @aganea !! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119257/new/ https://reviews.llvm.org/D119257 ___ cfe-commits mailing list cfe-commits@

[PATCH] D72404: [ThinLTO/FullLTO] Support Os and Oz

mehdi_amini added a comment. In D72404#3307623 , @aykevl wrote: > So, should all passes just look at the `optsize` and `minsize` attributes > instead of the `SizeLevel`? In other words, should > `PassManagerBuilder.SizeLevel` be removed and should passes

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

mehdi_amini added a comment. Seems like this should be added to canonicalization? The "push constants to the right hand side" is there already. I also don't understand the complexity of the implementation, I may need an example to understand why you're recursively operating on the producer ops

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

mehdi_amini added a comment. In D124750#3502228 , @srishti-pm wrote: > In D124750#3500748 , @mehdi_amini > wrote: > >> Seems like this should be added to canonicalization? The "push constants to >> the right ha

  1   2   3   4   >