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

[Mehdi AMINI via Phabricator via cfe-commits]

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.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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'll probably want to be 
> doing it, and I have another patch I'm working on which will make it that 
> much more obviously useful to have here. The middle-end can definitely figure 
> it out but it just seems like more work, later, so in the meantime we'd be 
> looking at more stuff.


I'm not asking where is it easier to do, but where does it make the most sense 
:)
Doing such in LLVM in general means catching more patterns (i.e. after 
inlining, etc.), and also catching it from multiple frontend. So in general I'm 
worried when I see optimizations implemented in the frontend  instead of the 
middle end.


Repository:
  rL LLVM

https://reviews.llvm.org/D49771



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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


Repository:
  rL LLVM

https://reviews.llvm.org/D49771



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33525: [ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM.

[Mehdi AMINI via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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://reviews.llvm.org/D28404

Files:
  cfe/trunk/include/clang/Driver/CC1Options.td
  cfe/trunk/include/clang/Frontend/CodeGenOptions.def
  cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp
  cfe/trunk/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
  cfe/trunk/lib/CodeGen/CodeGenModule.cpp
  cfe/trunk/lib/Frontend/CompilerInvocation.cpp
  cfe/trunk/test/CodeGen/aarch64-neon-2velem.c
  cfe/trunk/test/CodeGen/aarch64-neon-3v.c
  cfe/trunk/test/CodeGen/aarch64-neon-across.c
  cfe/trunk/test/CodeGen/aarch64-neon-extract.c
  cfe/trunk/test/CodeGen/aarch64-neon-fcvt-intrinsics.c
  cfe/trunk/test/CodeGen/aarch64-neon-fma.c
  cfe/trunk/test/CodeGen/aarch64-neon-intrinsics.c
  cfe/trunk/test/CodeGen/aarch64-neon-ldst-one.c
  cfe/trunk/test/CodeGen/aarch64-neon-misc.c
  cfe/trunk/test/CodeGen/aarch64-neon-perm.c
  cfe/trunk/test/CodeGen/aarch64-neon-scalar-copy.c
  cfe/trunk/test/CodeGen/aarch64-neon-scalar-x-indexed-elem.c
  cfe/trunk/test/CodeGen/aarch64-neon-shifts.c
  cfe/trunk/test/CodeGen/aarch64-neon-tbl.c
  cfe/trunk/test/CodeGen/aarch64-neon-vcombine.c
  cfe/trunk/test/CodeGen/aarch64-neon-vget-hilo.c
  cfe/trunk/test/CodeGen/aarch64-neon-vget.c
  cfe/trunk/test/CodeGen/aarch64-poly128.c
  cfe/trunk/test/CodeGen/aarch64-poly64.c
  cfe/trunk/test/CodeGen/address-safety-attr-kasan.cpp
  cfe/trunk/test/CodeGen/address-safety-attr.cpp
  cfe/trunk/test/CodeGen/arm-crc32.c
  cfe/trunk/test/CodeGen/arm-neon-directed-rounding.c
  cfe/trunk/test/CodeGen/arm-neon-fma.c
  cfe/trunk/test/CodeGen/arm-neon-numeric-maxmin.c
  cfe/trunk/test/CodeGen/arm-neon-shifts.c
  cfe/trunk/test/CodeGen/arm-neon-vcvtX.c
  cfe/trunk/test/CodeGen/arm-neon-vget.c
  cfe/trunk/test/CodeGen/arm64-crc32.c
  cfe/trunk/test/CodeGen/arm64-lanes.c
  cfe/trunk/test/CodeGen/arm64_vcopy.c
  cfe/trunk/test/CodeGen/arm64_vdupq_n_f64.c
  cfe/trunk/test/CodeGen/attr-coldhot.c
  cfe/trunk/test/CodeGen/attr-naked.c
  cfe/trunk/test/CodeGen/builtins-arm-exclusive.c
  cfe/trunk/test/CodeGen/builtins-arm.c
  cfe/trunk/test/CodeGen/builtins-arm64.c
  cfe/trunk/test/CodeGen/noduplicate-cxx11-test.cpp
  cfe/trunk/test/CodeGen/pragma-weak.c
  cfe/trunk/test/CodeGen/unwind-attr.c
  cfe/trunk/test/CodeGenCXX/apple-kext-indirect-virtual-dtor-call.cpp
  cfe/trunk/test/CodeGenCXX/apple-kext-no-staticinit-section.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-global-ctor-dtor.cpp
  cfe/trunk/test/CodeGenCXX/optnone-templates.cpp
  cfe/trunk/test/CodeGenCXX/static-init-wasm.cpp
  cfe/trunk/test/CodeGenCXX/thunks.cpp
  cfe/trunk/test/CodeGenObjC/gnu-exceptions.m
  cfe/trunk/test/CodeGenOpenCL/amdgpu-attrs.cl
  cfe/trunk/test/Driver/darwin-iphone-defaults.m

Index: cfe/trunk/include/clang/Driver/CC1Options.td
===
--- cfe/trunk/include/clang/Driver/CC1Options.td
+++ cfe/trunk/include/clang/Driver/CC1Options.td
@@ -172,6 +172,8 @@
 def disable_lifetimemarkers : Flag<["-"], "disable-lifetime-markers">,
   HelpText<"Disable lifetime-markers emission even when optimizations are "
"enabled">;
+def disable_O0_optnone : Flag<["-"], "disable-O0-optnone">,
+  HelpText<"Disable adding the optnone attribute to functions at O0">;
 def disable_red_zone : Flag<["-"], "disable-red-zone">,
   HelpText<"Do not emit code that uses the red zone.">;
 def dwarf_column_info : Flag<["-"], "dwarf-column-info">,
Index: cfe/trunk/include/clang/Frontend/CodeGenOptions.def
===
--- cfe/trunk/include/clang/Frontend/CodeGenOptions.def
+++ cfe/trunk/include/clang/Frontend/CodeGenOptions.def
@@ -53,6 +53,7 @@
  ///< the pristine IR generated by the
  ///< frontend.
 CODEGENOPT(DisableLifetimeMarkers, 1, 0) ///< Don't emit any lifetime markers
+CODEGENOPT(DisableO0ImplyOptNone , 1, 0) ///< Don't annonate function with optnone at O0
 CODEGENOPT(ExperimentalNewPassManager, 1, 0) ///< Enables the new, experimental
  ///< pass manager.
 CODEGENOPT(DisableRedZone, 1, 0) ///< Set when -mno-red-zone is enabled.
Index: cfe/trunk/test/CodeGenCXX/optnone-templates.cpp
===
--- cfe/trunk/test/CodeGenCXX/optnone-templates.cpp
+++ cfe/trunk/test/CodeGenCXX/optnone-templates.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -triple %itanium_abi_triple -std=c++11 -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple %itanium_abi_triple -std=c++11 -disable-O0-optnone -emit-llvm -o - | FileCheck %s
 
 // Test optnone on template instantiations.
 
Index: cfe/trunk/test/CodeGenCXX/debug-info-global-ctor-dtor.cpp
==

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

[Mehdi AMINI via Phabricator via cfe-commits]

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/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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

https://reviews.llvm.org/D33976



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 be wonderful if smb 
> could look at this diff (which, besides the fix, adds tests)


I'll CC you on the bug when I get time to reduce the test case and file the bug.


Repository:
  rL LLVM

https://reviews.llvm.org/D33976



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 separate issue.
> >  Thanks for adding @ahatanak and @arphaman, that would be wonderful if smb 
> > could look at this diff (which, besides the fix, adds tests)
>
>
> I'll CC you on the bug when I get time to reduce the test case and file the 
> bug.


I finally got to reduce it: https://bugs.llvm.org/show_bug.cgi?id=33447


Repository:
  rL LLVM

https://reviews.llvm.org/D33976



___
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 via Phabricator via cfe-commits]

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 think some code/logic needs to be changed in LLVM first:

  bool LTOModule::isThinLTO() {
// Right now the detection is only based on the summary presence. We may 
want
// to add a dedicated flag at some point.




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 via Phabricator via cfe-commits]

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 via Phabricator via cfe-commits]

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) {
-  return ArgUse.first == U.first && ArgUse.second != U.second;
-}) != I->second.end()) {
+std::find_if(I->second.begin(), I->second.end(),
+ [&](const MacroArgUse &U) {

Nit: `llvm::find_if` allows to skip begin/end as `llvm::find_if(I->second, [] 
...`



Comment at: test/FixIt/fixit-format-darwin.m:71
+
+void radar33447() {
+  Outer2("test 8: %s", getNSInteger());  

I think you mean `pr` instead of `radar` ;)


Repository:
  rL LLVM

https://reviews.llvm.org/D34268



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 it usual?
> It does seem a little odd, but it's in the same place as the other 
> linker-specific options above. I was also imagining that we would eventually 
> make this into a clang driver option (similar to the cache location as you 
> mentioned in D30522). At that point the clang docs would indeed be the right 
> place to document it.
Fair!
I'll let Teresa look and approve.


https://reviews.llvm.org/D34546



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57330: Adjust documentation for git migration.

[Mehdi AMINI via Phabricator via cfe-commits]

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



Comment at: libcxx/docs/BuildingLibcxx.rst:57
   $ cd where-you-want-libcxx-to-live
-  $ # Check out llvm, libc++ and libc++abi.
-  $ ``svn co http://llvm.org/svn/llvm-project/llvm/trunk llvm``
-  $ ``svn co http://llvm.org/svn/llvm-project/libcxx/trunk libcxx``
-  $ ``svn co http://llvm.org/svn/llvm-project/libcxxabi/trunk libcxxabi``
+  $ # Check out the sources (includes everything, but we'll only use libcxx)
+  $ ``git clone https://github.com/llvm/llvm-project.git``

Wonder if it is worth mentioning somewhere how to sparse-checkout?



Comment at: libcxxabi/www/index.html:86
   mkdir build && cd build
-  cmake .. # on linux you may need to prefix with CC=clang 
CXX=clang++
+  cmake -DLLVM_ENABLE_PROJECTS=libcxxabi ../llvm # on linux you may 
need to prefix with CC=clang CXX=clang++
   make

Do you now if prefixing with CC is equivalent to -DCMAKE_C_COMPILER?



Comment at: lld/docs/getting_started.rst:71
+ $ cd llvm-project/build (out of source build required)
+ $ cmake -G "Visual Studio 11" ../llvm
 

Missing -DLLVM_ENABLE_PROJECTS=lld here I believe


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57330/new/

https://reviews.llvm.org/D57330



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57330: Adjust documentation for git migration.

[Mehdi AMINI via Phabricator via cfe-commits]

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 won't touch anything outside of the build directory: i.e. the build 
directory itself is hermetic, self-contained, and can be destroyed without 
impacting anything else.
(I agree with you that polluting `git status` is annoying, and personally I 
usually have my build directories in parallel with the repo).


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57330/new/

https://reviews.llvm.org/D57330



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57330: Adjust documentation for git migration.

[Mehdi AMINI via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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_features:
+config.unsupported = True

thakis wrote:
> Did you mean `not in` here? I don't understand why we want to skip this tests 
> is availability annotations are present.
Thanks for looking into this, unfortunately I don't remember the context from 2 
years ago.  It may have been a typo indeed.

@ldionne and @Bigcheese  may be able to help here!


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D31739/new/

https://reviews.llvm.org/D31739



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 list of passes 
for a given opt level?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61022/new/

https://reviews.llvm.org/D61022



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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:
> > > tejohnson wrote:
> > > > mehdi_amini wrote:
> > > > > This is intended? I'm surprised the two PMs don't have the same list 
> > > > > of passes for a given opt level?
> > > > I'm really not sure. I did compare the post-link LTO pipelines of both 
> > > > PMs at O0/O1/O2 and confirmed that the old PM is doing many more passes 
> > > > than the new PM at O1. Probably a question for @chandlerc ? In any 
> > > > case, I didn't want to address it here since it is orthogonal.
> > > Some more info:
> > > 
> > > This is the regular LTO post-link pipeline, ThinLTO post-link pipeline 
> > > uses essentially the same as a normal opt pipeline so would be consistent 
> > > at -O1.
> > > 
> > > The optimization missing from the new PM regular LTO post link pipeline 
> > > that affects this test is SimplifyCFG. This and a few other optimizations 
> > > are added in the old PM at O1 via 
> > > PassManagerBuilder::addLateLTOOptimizationPasses. Note that 
> > > PassManagerBuilder::addLTOOptimizationPasses does exit early at -O1 
> > > (similar to where we exit early in the new PM for the regular LTO post 
> > > link compilation). But the "late" LTO optimization passes are added 
> > > unconditionally above -O0. Is this correct/desired? There isn't an 
> > > equivalent in the new PM.
> > I don't think it was intentional, but I'm not sure I would directly 
> > replicate it if it requires adding an entirely new customization point. Is 
> > their some simpler way of getting equivalent results at O1?
> Yeah we can presumably just add those optimizations to the -O1 early exit 
> path in PassBuilder::buildLTODefaultPipeline. I can send a patch (but would 
> like to get this one in as it is a bugfix and orthogonal).
(I fully agree it is orthogonal, I just spotted this as a separate bug indeed, 
thanks for fixing!)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61022/new/

https://reviews.llvm.org/D61022



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 consensus,
> >
> >
> > Based on three comments in a revision? Seems strange to me.
> >  I don't really care about this, so do whatever you want, but I would 
> > expect that "consensus" means an actual wider discussion (i.e. llvm-dev + 
> > cfe-dev).
>
>
> Please cite said discussion for when you added this, as requested above.


Sorry, I don't have time to do archeology for you right now. But this is beside 
the point: your patch is changing a 2 years status quo, so my take on it is 
that it is *on you* to build the consensus to change this (maybe the consensus 
exists, I don't know, but this Phabricator diff alone seems quite light to 
demonstrate evidence of it).

> Else, I think this has seen more discussion than the change that is undoing. 
> It also has the support of several folks very actively working on clang and 
> clang-tools-extra.

Again: I have no incentive to weigh one way or another with respect to what is 
the right way forward for clang-tools-extra, so I don't care what happens here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58157/new/

https://reviews.llvm.org/D58157



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 more options, today.


Sure, looks fine. The cost of reversing it isn't that high either (updating a 
bunch of bot configurations).
It'd still be nice to identify the end goal with most cfe people (i.e. even if 
you land this right now, discussing the desired layout in the monorepo)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58157/new/

https://reviews.llvm.org/D58157



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 now, discussing the desired layout in the monorepo)
>
>
> I sent off http://lists.llvm.org/pipermail/cfe-dev/2019-February/061290.html, 
> so the larger community is aware of this change. I don't have much to add to 
> a larger discussion about possible code reorganizations, but I suppose folks 
> may come forward to express their opinions.


Thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58157/new/

https://reviews.llvm.org/D58157



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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

https://reviews.llvm.org/D34992

Files:
  cfe/trunk/lib/CodeGen/CodeGenModule.cpp
  cfe/trunk/test/CodeGenCXX/cxx11-extern-constexpr.cpp


Index: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
===
--- cfe/trunk/lib/CodeGen/CodeGenModule.cpp
+++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp
@@ -2437,6 +2437,28 @@
 D->getType().isConstant(Context) &&
 isExternallyVisible(D->getLinkageAndVisibility().getLinkage()))
   GV->setSection(".cp.rodata");
+
+// Check if we a have a const declaration with an initializer, we may be
+// able to emit it as available_externally to expose it's value to the
+// optimizer.
+if (Context.getLangOpts().CPlusPlus && GV->hasExternalLinkage() &&
+D->getType().isConstQualified() && !GV->hasInitializer() &&
+!D->hasDefinition() && D->hasInit() && !D->hasAttr()) {
+  const auto *Record =
+  Context.getBaseElementType(D->getType())->getAsCXXRecordDecl();
+  bool HasMutableFields = Record && Record->hasMutableFields();
+  if (!HasMutableFields) {
+const VarDecl *InitDecl;
+const Expr *InitExpr = D->getAnyInitializer(InitDecl);
+if (InitExpr) {
+  GV->setConstant(true);
+  GV->setLinkage(llvm::GlobalValue::AvailableExternallyLinkage);
+  ConstantEmitter emitter(*this);
+  GV->setInitializer(emitter.tryEmitForInitializer(*InitDecl));
+  emitter.finalize(GV);
+}
+  }
+}
   }
 
   auto ExpectedAS =
Index: cfe/trunk/test/CodeGenCXX/cxx11-extern-constexpr.cpp
===
--- cfe/trunk/test/CodeGenCXX/cxx11-extern-constexpr.cpp
+++ cfe/trunk/test/CodeGenCXX/cxx11-extern-constexpr.cpp
@@ -0,0 +1,55 @@
+// RUN: %clang_cc1 -std=c++11 %s -emit-llvm -o - -triple x86_64-linux-gnu | 
FileCheck %s --check-prefix=CHECK --check-prefix=CXX11
+// RUN: %clang_cc1 -std=c++1z %s -emit-llvm -o - -triple x86_64-linux-gnu | 
FileCheck %s --check-prefix=CHECK --check-prefix=CXX17
+
+struct A {
+  static const int Foo = 123;
+};
+// CHECK: @_ZN1A3FooE = constant i32 123, align 4
+const int *p = &A::Foo; // emit available_externally
+const int A::Foo;   // convert to full definition
+
+struct Bar {
+  int b;
+};
+
+struct MutableBar {
+  mutable int b;
+};
+
+struct Foo {
+  // CXX11: @_ZN3Foo21ConstexprStaticMemberE = available_externally constant 
i32 42,
+  // CXX17: @_ZN3Foo21ConstexprStaticMemberE = linkonce_odr constant i32 42,
+  static constexpr int ConstexprStaticMember = 42;
+  // CHECK: @_ZN3Foo17ConstStaticMemberE = available_externally constant i32 
43,
+  static const int ConstStaticMember = 43;
+
+  // CXX11: @_ZN3Foo23ConstStaticStructMemberE = available_externally constant 
%struct.Bar { i32 44 },
+  // CXX17: @_ZN3Foo23ConstStaticStructMemberE = linkonce_odr constant 
%struct.Bar { i32 44 },
+  static constexpr Bar ConstStaticStructMember = {44};
+
+  // CXX11: @_ZN3Foo34ConstexprStaticMutableStructMemberE = external global 
%struct.MutableBar,
+  // CXX17: @_ZN3Foo34ConstexprStaticMutableStructMemberE = linkonce_odr 
global %struct.MutableBar { i32 45 },
+  static constexpr MutableBar ConstexprStaticMutableStructMember = {45};
+};
+// CHECK: @_ZL15ConstStaticexpr = internal constant i32 46,
+static constexpr int ConstStaticexpr = 46;
+// CHECK: @_ZL9ConstExpr = internal constant i32 46, align 4
+static const int ConstExpr = 46;
+
+// CHECK: @_ZL21ConstexprStaticStruct = internal constant %struct.Bar { i32 47 
},
+static constexpr Bar ConstexprStaticStruct = {47};
+
+// CHECK: @_ZL28ConstexprStaticMutableStruct = internal global 
%struct.MutableBar { i32 48 },
+static constexpr MutableBar ConstexprStaticMutableStruct = {48};
+
+void use(const int &);
+void foo() {
+  use(Foo::ConstexprStaticMember);
+  use(Foo::ConstStaticMember);
+  use(Foo::ConstStaticStructMember.b);
+  use(Foo::ConstexprStaticMutableStructMember.b);
+  use(ConstStaticexpr);
+  use(ConstExpr);
+  use(ConstexprStaticStruct.b);
+  use(ConstexprStaticMutableStruct.b);
+}


Index: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
===
--- cfe/trunk/lib/CodeGen/CodeGenModule.cpp
+++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp
@@ -2437,6 +2437,28 @@
 D->getType().isConstant(Context) &&
 isExternallyVisible(D->getLinkageAndVisibility().getLinkage()))
   GV->setSection(".cp.rodata");
+
+// Check if we a have a const declaration with an initializer, we may be
+// able to emit it as available_externally to expose it's value to the
+// optimizer.
+if (Context.getLangOpts().CPlusPlus && GV->hasExterna

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

[Mehdi AMINI via Phabricator via cfe-commits]

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/CodeGenModule.cpp:2448-2451
+if (InitExpr) {
+  GV->setLinkage(llvm::GlobalValue::AvailableExternallyLinkage);
+  GV->setInitializer(EmitConstantInit(*InitDecl));
+}

rsmith wrote:
> mehdi_amini wrote:
> > rsmith wrote:
> > > In order for this transformation to be correct, you need to know that the 
> > > variable has static initialization, which means that it needs to formally 
> > > have a constant initializer. You can use `D->isInitKnownICE()` to check 
> > > this.
> > Done! But couldn't find how to express it as a test-case though.
> You'd need to use an initializer that we can constant-fold, such as:
> 
> ```
> struct A {
>   static const int n;
> };
> bool check() {
>   assert(A::n == 0 && "already initialized!");
>   return true;
> }
> const int A::n = (check() || true) ? 1 : 2;
> ```
But this wouldn't be an "available_externally". We need the initializer in the 
class definition.

I tried multiple variant but couldn't get one that compile:

```
struct B {
  static bool check() {
if (B::n == 0) return false;
return true;
  }
  // error: in-class initializer for static data member is not a constant 
expression
  static const int n = (B::check() || true) ? 1 : 2;
};
```

```
struct B {
  static constexpr bool check() {
// "error: constexpr function never produces a constant expression"
return  (B::n == 0 || true) ? false : true;
  }
//"note: initializer of 'n' is not a constant expression"
  static constexpr int n = (B::check() || true) ? 1 : 2;
};
```






Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2445
+!D->hasDefinition() && D->hasInit() &&
+/* C++17 static constexpr are inlined */ !D->isInline() &&
+!D->hasAttr() && D->isInitKnownICE()) {

rsmith wrote:
> Do we need to special-case this? Such declarations are definitions.
We don't! Simplified after rebasing and adapting after John's changes.


Repository:
  rL LLVM

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] D63976: Allow clang -Os and -Oz to work with -flto and lld

[Mehdi AMINI via Phabricator via cfe-commits]

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 invoked by the linker.

Phase 1: compile to bitcode
===

clang++ -Os -flto foo.c -o foo.o
clang++ -O2 -flto bar.c -o bar.o

Phase 2: LTO and CodeGen


clang++  bar.o foo.o

When compiling foo.c, we use Os which add a function attribute to make each 
function in foo.o as such. Functions in bar.o won't have the same attributes.
During LTO we merge everything but functions keep their attributes, the 
optimization passes can adjust their heuristics to honor the fact that 
functions from foo.c will be "optimized for size" and the function from bar.c 
will be optimized "normally".

Specifying an optimization level for the LTO phase is a also bit awkward since 
the LTO optimization pipeline is already very different from the non-LTO one: 
https://github.com/llvm/llvm-project/blob/master/llvm/lib/Transforms/IPO/PassManagerBuilder.cpp#L1012

What remains are CodeGen heuristics, and there might still have some global 
flags in use: 
https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/Support/CodeGen.h#L51
 (note nothing specific to Os/Oz though), still in use for example for 
generating FMAs on AArch64 apparently: 
https://github.com/llvm/llvm-project/blob/master/llvm/lib/Target/AArch64/AArch64SelectionDAGInfo.cpp#L55

This is why specifying Oz/Os during LTO can be very confusing for the user: it 
would change very few things in the process without actually making function 
from bar.c having the right function attributes (nothing would override the 
lack of attribute as far as I know): `clang++ -flto -Oz bar.o foo.o` would 
*not* add the Oz annotation on functions defined in bar.o.

> I'm curious why we would want to force -O2/-O3 instead of just allowing 
> -Os/-Oz to be used with LTO.

So I hope it is more clear that I don't think we're forcing O2 
/O3 
 on LTO users, but it isn't obvious 
to expose a consistent UI for these flags with respect to LTO without being 
surprising to the user in some way.
(do you know that LTO use to be -O4?)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63976/new/

https://reviews.llvm.org/D63976



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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`/`-O3`?

What you're invoking is the clang driver, it will drive the process and spawn 
multiple subprocess for each phase (this contributes to the UI challenge). Your 
"funny" command line will detect that it needs to spawn a clang instance for 
the compile phase of b.c to get an object file before invoking the linker on 
the temporary b.o and the a.o you provide on the input. To add some complexity, 
the exact behavior depends on the platform (Linux vs MacOS) and possible which 
linker is in use (-fuse-ld=lld).

But yes this patch changes the driver so that only the invocation of the linker 
is changed. Your previous "funny" command line would still yield Oz when 
compiling b.c to b.o before invoking the linker on b.o and a.o with O3 
.

(I don't know if this change is the best solution, I just provide my 
understanding of the situation :))


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63976/new/

https://reviews.llvm.org/D63976



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 why we have three different 
> options in the first place). Adding an Os and Oz to lld's LTO option seems 
> like a good idea, but they should be mapped to their corresponding features.


It shouldn't Matt




Comment at: llvm-9.0.0-20190629/clang/lib/Driver/ToolChains/CommonArgs.cpp:395
+  if(OOpt == "s" || OOpt == "z")
+OOpt = "3";
+}

tejohnson wrote:
> Os/Oz are closer to O2 than O3 (which allows much more aggressive code size 
> increasing optimizations).
> 
> Better though to add a size level to the LTO::Config, teach lld to pass it 
> through properly, then using the LTO Config to set the SizeLevel in the old 
> PM and the PassBuilder::OptimizationLevel in the new PM when setting up the 
> LTO backend pipelines, similar to how CodeGenLevel.OptimizeSize is handled in 
> clang (BackendUtil.cpp).
> 
> My concern is that silently mapping Os/Oz to do something different than in 
> the non-LTO pipeline is going to end up more confusing than the current error 
> (which isn't good either, but at least makes it clear that it isn't 
> supported).
Using O2 makes sense to me. 
Beyond this does it matter much? Isn't the important part of Os/Oz  carried 
through a function attribute?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63976/new/

https://reviews.llvm.org/D63976



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 of interpretation of what is valid to do or not changes according to this 
module flag.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63932/new/

https://reviews.llvm.org/D63932



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 what is valid to do or not changes according 
> > to this module flag.
>
> I think the original version was better from that perspective - most compiler 
> passes only need to check for one value of the attribute, and only the linker 
> needed to care about the difference between link-unit and public visibility.


The *linker* needed to care about the difference is fine, but that's different 
from a *compiler pass* during LTO. In general it is an important design point 
in LLVM so far to encode these information in the IR (for instance the linkage 
type of globals would be changed and not the interpretation of these with a 
flag).

> Do you think we should go back to that design, or do you have a different 
> idea?

I didn't review the original version, but pcc@ did apparently, so I'm willing 
to trust them on what's the best way forward here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63932/new/

https://reviews.llvm.org/D63932



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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, FramePointerKind, 2, FramePointerKind::None) /// 
frame-pointer: all,non-leaf,none

Nit: indentation is off here.



Comment at: clang/include/clang/Basic/CodeGenOptions.def:341
+CODEGENOPT(RelocateWithSymbols, 1, 0)
+
 /// Whether we should use the undefined behaviour optimization for control flow

Can you add a doc here? (possibly referring to somewhere else if it is 
extensively documented elsewhere?)



Comment at: clang/include/clang/Driver/Options.td:1873
+def fbasicblock_sections_EQ : Joined<["-"], "fbasicblock-sections=">, 
Group, Flags<[CC1Option, CC1AsOption]>,
+  HelpText<"Place each function's basic blocks in unique sections (ELF Only) : 
all | labels | none | ">;
 def fdata_sections : Flag <["-"], "fdata-sections">, Group,

Is the "labels" options dependent/related to the previous -fpropeller-label one?



Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:348
+Twine NewName = Section + Name;
+Fn->setSection(NewName.str());
+  }

Is this related to these new option? It this change the behavior of 
-ffunction-sections?



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1032
 }
-
+  }
   return Out.str();

I agree with the improvement, but as nit this isn't related to the current 
patch or even in a function you're otherwise touching. (it creates an extra 
hunk in the review)



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1100
+  // internal linkage functions, to differentiate the symbols across
+  // modules.
+  if (getCodeGenOpts().UniqueInternalFuncNames &&

What happens in case of conflict? (maybe clarify in the comment)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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/28/206


I'm wondering why they never pushed all the way for a `-std=linux-c` flag 
instead, and produced a documentation with the behavior they want with respect 
to the base C standard they align on.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64742/new/

https://reviews.llvm.org/D64742



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69498: IR: Invert convergent attribute handling

[Mehdi AMINI via Phabricator via cfe-commits]

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 frontend projects have consistently 
> run into problems related to this.

Can you clarify what kind of problems? Do you have concrete examples?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69498/new/

https://reviews.llvm.org/D69498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 LLVM to do this. Is there a way this can 
> be done in the call to `find_package(LLVM)` instead?
(Doc for the usual way to integrate LLVM: 
https://llvm.org/docs/CMake.html#embedding-llvm-in-your-project )


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79219/new/

https://reviews.llvm.org/D79219



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 way this can be 
done in the call to `find_package(LLVM)` instead?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79219/new/

https://reviews.llvm.org/D79219



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 where it has been problematic at the moment?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77683/new/

https://reviews.llvm.org/D77683



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 intent, I just feel that 
adding this sentence here won't help and does not fit in an existing "process".

And you're adding this after "If there is likely to be uncertainty, you should 
default to getting a patch reviewed prior to commit", I don't see why the 
addition is useful here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77683/new/

https://reviews.llvm.org/D77683



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 generic 
utility in Support to introduce a dependency on IR.

I speculatively reverted in 
https://github.com/llvm/llvm-project/commit/57d2d48399b63c0ef1dda490fdaf28efbb80c2c0
 while we can figure it out.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77341/new/

https://reviews.llvm.org/D77341



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77341/new/

https://reviews.llvm.org/D77341



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 violation to me here: I wouldn't expect a 
> > generic utility in Support to introduce a dependency on IR.
> > 
> > I speculatively reverted in 
> > https://github.com/llvm/llvm-project/commit/57d2d48399b63c0ef1dda490fdaf28efbb80c2c0
> >  while we can figure it out.
> Yep - agreed on the layering violation. Thanks for the catch/revert!
> 
> I /think/ CFGDiff.h is independent of IR & shouldn't include IR/CFG.h and 
> then can move into Support. Ah, yeah, when I did 
> 30804d0a3fb23325c4b455108e59d00213b83891 & related work, that's what made it 
> independent of IR.
> 
> Moved it over to Support in a838aadae3f20b9644e2ad553d3d558a91fd8fd7
Thanks! Re-landed in 0445c64998d14b81f0d3a3182011fc5eae47fa71


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77341/new/

https://reviews.llvm.org/D77341



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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:
> > > This looks like a layering violation to me here: I wouldn't expect a 
> > > generic utility in Support to introduce a dependency on IR.
> > > 
> > > I speculatively reverted in 
> > > https://github.com/llvm/llvm-project/commit/57d2d48399b63c0ef1dda490fdaf28efbb80c2c0
> > >  while we can figure it out.
> > Yep - agreed on the layering violation. Thanks for the catch/revert!
> > 
> > I /think/ CFGDiff.h is independent of IR & shouldn't include IR/CFG.h and 
> > then can move into Support. Ah, yeah, when I did 
> > 30804d0a3fb23325c4b455108e59d00213b83891 & related work, that's what made 
> > it independent of IR.
> > 
> > Moved it over to Support in a838aadae3f20b9644e2ad553d3d558a91fd8fd7
> Thanks! Re-landed in 0445c64998d14b81f0d3a3182011fc5eae47fa71
Actually reverted again as it breaks the MLIR build in a different way, I won't 
be able to look into this in more details right now.

(I ran `ninja check` but I forgot that `check` is not `check-all`...)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77341/new/

https://reviews.llvm.org/D77341



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 accepted and ready to land.

lgtm


This reverts commit 60c642e74be6af86906d9f3d982728be7bd4329f 
.

This patch is making the TLI "closed" for a predefined set of VecLib
while at the moment it is extensible for anyone to customize when using
LLVM as a library.
Reverting while we figure out a way to re-land it without losing the
generality of the current API.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77925

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGen/libcalls-veclib.c
  llvm/include/llvm/Analysis/TargetLibraryInfo.h
  llvm/lib/Analysis/InlineCost.cpp
  llvm/lib/Analysis/TargetLibraryInfo.cpp
  llvm/test/Transforms/Inline/inline-no-builtin-compatible.ll
  llvm/test/Transforms/Inline/veclib-compat.ll

Index: llvm/test/Transforms/Inline/veclib-compat.ll
===
--- llvm/test/Transforms/Inline/veclib-compat.ll
+++ /dev/null
@@ -1,48 +0,0 @@
-; RUN: opt < %s -inline -inline-caller-superset-tli=true -S | FileCheck %s --check-prefixes=COMMON
-; RUN: opt < %s -passes='cgscc(inline)' -inline-caller-superset-tli=true -S | FileCheck %s --check-prefixes=COMMON
-; RUN: opt < %s -inline -inline-caller-superset-tli=false -S | FileCheck %s --check-prefixes=NOSUPERSET,COMMON
-; RUN: opt < %s -passes='cgscc(inline)' -inline-caller-superset-tli=false -S | FileCheck %s --check-prefixes=NOSUPERSET,COMMON
-
-
-
-define i32 @callee_svml(i8 %X) #0 {
-entry:
-  ret i32 1
-}
-
-define i32 @callee_massv(i8 %X) #1 {
-entry:
-  ret i32 1
-}
-
-define i32 @callee_nolibrary(i8 %X) {
-entry:
-  ret i32 1
-}
-
-define i32 @caller_svml() #0 {
-; COMMON-LABEL: define i32 @caller_svml()
-entry:
-  %rslt = call i32 @callee_massv(i8 123)
-; COMMON: call i32 @callee_massv
-  %tmp1 = call i32 @callee_nolibrary(i8 123)
-; NOSUPERSET: call i32 @callee_nolibrary
-  %tmp2 = call i32 @callee_svml(i8 123)
-; COMMON-NOT: call
-  ret i32 %rslt
-}
-
-define i32 @caller_nolibrary() {
-; COMMON-LABEL: define i32 @caller_nolibrary()
-entry:
-  %rslt = call i32 @callee_svml(i8 123)
-; COMMON: call i32 @callee_svml
-  %tmp1 = call i32 @callee_massv(i8 123)
-; COMMON: call i32 @callee_massv
-  %tmp2 = call i32 @callee_nolibrary(i8 123)
-; COMMON-NOT: call
-  ret i32 %rslt
-}
-
-attributes #0 = { "veclib"="SVML" }
-attributes #1 = { "veclib"="MASSV" }
\ No newline at end of file
Index: llvm/test/Transforms/Inline/inline-no-builtin-compatible.ll
===
--- llvm/test/Transforms/Inline/inline-no-builtin-compatible.ll
+++ llvm/test/Transforms/Inline/inline-no-builtin-compatible.ll
@@ -3,8 +3,8 @@
 ; RUN: opt < %s -mtriple=x86_64-unknown-linux-gnu -S -passes='cgscc(inline)' | FileCheck %s
 
 ; Make sure we don't inline callees into a caller with a superset of the
-; no builtin attributes when -inline-caller-superset-tli=false.
-; RUN: opt < %s -inline-caller-superset-tli=false -mtriple=x86_64-unknown-linux-gnu -S -passes='cgscc(inline)' | FileCheck %s --check-prefix=NOSUPERSET
+; no builtin attributes when -inline-caller-superset-nobuiltin=false.
+; RUN: opt < %s -inline-caller-superset-nobuiltin=false -mtriple=x86_64-unknown-linux-gnu -S -passes='cgscc(inline)' | FileCheck %s --check-prefix=NOSUPERSET
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
Index: llvm/lib/Analysis/TargetLibraryInfo.cpp
===
--- llvm/lib/Analysis/TargetLibraryInfo.cpp
+++ llvm/lib/Analysis/TargetLibraryInfo.cpp
@@ -550,7 +550,7 @@
 TLI.setUnavailable(LibFunc_nvvm_reflect);
   }
 
-  TLI.addAllVectorizableFunctions();
+  TLI.addVectorizableFunctionsFromVecLib(ClVectorLibrary);
 }
 
 TargetLibraryInfoImpl::TargetLibraryInfoImpl() {
@@ -572,8 +572,8 @@
   ShouldExtI32Return(TLI.ShouldExtI32Return),
   ShouldSignExtI32Param(TLI.ShouldSignExtI32Param) {
   memcpy(AvailableArray, TLI.AvailableArray, sizeof(AvailableArray));
-  for (unsigned i = 0; i < NumVecLibs; i++)
-VecLibDescs[i] = TLI.VecLibDescs[i];
+  VectorDescs = TLI.VectorDescs;
+  ScalarDescs = TLI.ScalarDescs;
 }
 
 TargetLibraryInfoImpl::TargetLibraryInfoImpl(TargetLibraryInfoImpl &&TLI)
@@ -583,8 +583,8 @@
   ShouldSignExtI32Param(TLI.ShouldSignExtI32Param) {
   std::move(std::begin(TLI.AvailableArray), std::end(TLI.AvailableArray),
 AvailableArray);
-  for (unsigned i = 0; i < NumVecLibs; i++)
-VecLibDescs[i] = TLI.VecLibDescs[i];
+  VectorDescs = TLI.VectorDescs;
+  ScalarDescs = TLI.ScalarDescs;
 }
 
 TargetLibraryInfoImpl &T

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 patch to preserve the current ability 
but I wanted to check first (and in the meantime I reverted in temporarily in 
https://reviews.llvm.org/D77925 to avoid the feature regression).

At the moment the place where you seem to use this knowledge is with the `enum 
VectorLibrary`  in the `TargetLibraryInfoImpl` class, and the `VecLibDescs` 
array which statically contains the known VecLib.
It seems to me that if we replace this enum with a string instead to identify 
the VecLib everything should still hold together and this would fit with minor 
changes to this path. The `VecLibDescs` could just be a 
`StringMap` in this case.

That was a third-party (in my case the XLA compiler) can still register its own 
"XLA" VecLib and add all the descriptors.

How does it sound?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77632/new/

https://reviews.llvm.org/D77632



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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.org/D77925/new/

https://reviews.llvm.org/D77925

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGen/libcalls-veclib.c
  llvm/include/llvm/Analysis/TargetLibraryInfo.h
  llvm/lib/Analysis/InlineCost.cpp
  llvm/lib/Analysis/TargetLibraryInfo.cpp
  llvm/test/Transforms/Inline/inline-no-builtin-compatible.ll
  llvm/test/Transforms/Inline/veclib-compat.ll

Index: llvm/test/Transforms/Inline/veclib-compat.ll
===
--- llvm/test/Transforms/Inline/veclib-compat.ll
+++ /dev/null
@@ -1,48 +0,0 @@
-; RUN: opt < %s -inline -inline-caller-superset-tli=true -S | FileCheck %s --check-prefixes=COMMON
-; RUN: opt < %s -passes='cgscc(inline)' -inline-caller-superset-tli=true -S | FileCheck %s --check-prefixes=COMMON
-; RUN: opt < %s -inline -inline-caller-superset-tli=false -S | FileCheck %s --check-prefixes=NOSUPERSET,COMMON
-; RUN: opt < %s -passes='cgscc(inline)' -inline-caller-superset-tli=false -S | FileCheck %s --check-prefixes=NOSUPERSET,COMMON
-
-
-
-define i32 @callee_svml(i8 %X) #0 {
-entry:
-  ret i32 1
-}
-
-define i32 @callee_massv(i8 %X) #1 {
-entry:
-  ret i32 1
-}
-
-define i32 @callee_nolibrary(i8 %X) {
-entry:
-  ret i32 1
-}
-
-define i32 @caller_svml() #0 {
-; COMMON-LABEL: define i32 @caller_svml()
-entry:
-  %rslt = call i32 @callee_massv(i8 123)
-; COMMON: call i32 @callee_massv
-  %tmp1 = call i32 @callee_nolibrary(i8 123)
-; NOSUPERSET: call i32 @callee_nolibrary
-  %tmp2 = call i32 @callee_svml(i8 123)
-; COMMON-NOT: call
-  ret i32 %rslt
-}
-
-define i32 @caller_nolibrary() {
-; COMMON-LABEL: define i32 @caller_nolibrary()
-entry:
-  %rslt = call i32 @callee_svml(i8 123)
-; COMMON: call i32 @callee_svml
-  %tmp1 = call i32 @callee_massv(i8 123)
-; COMMON: call i32 @callee_massv
-  %tmp2 = call i32 @callee_nolibrary(i8 123)
-; COMMON-NOT: call
-  ret i32 %rslt
-}
-
-attributes #0 = { "veclib"="SVML" }
-attributes #1 = { "veclib"="MASSV" }
\ No newline at end of file
Index: llvm/test/Transforms/Inline/inline-no-builtin-compatible.ll
===
--- llvm/test/Transforms/Inline/inline-no-builtin-compatible.ll
+++ llvm/test/Transforms/Inline/inline-no-builtin-compatible.ll
@@ -3,8 +3,8 @@
 ; RUN: opt < %s -mtriple=x86_64-unknown-linux-gnu -S -passes='cgscc(inline)' | FileCheck %s
 
 ; Make sure we don't inline callees into a caller with a superset of the
-; no builtin attributes when -inline-caller-superset-tli=false.
-; RUN: opt < %s -inline-caller-superset-tli=false -mtriple=x86_64-unknown-linux-gnu -S -passes='cgscc(inline)' | FileCheck %s --check-prefix=NOSUPERSET
+; no builtin attributes when -inline-caller-superset-nobuiltin=false.
+; RUN: opt < %s -inline-caller-superset-nobuiltin=false -mtriple=x86_64-unknown-linux-gnu -S -passes='cgscc(inline)' | FileCheck %s --check-prefix=NOSUPERSET
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
Index: llvm/lib/Analysis/TargetLibraryInfo.cpp
===
--- llvm/lib/Analysis/TargetLibraryInfo.cpp
+++ llvm/lib/Analysis/TargetLibraryInfo.cpp
@@ -550,7 +550,7 @@
 TLI.setUnavailable(LibFunc_nvvm_reflect);
   }
 
-  TLI.addAllVectorizableFunctions();
+  TLI.addVectorizableFunctionsFromVecLib(ClVectorLibrary);
 }
 
 TargetLibraryInfoImpl::TargetLibraryInfoImpl() {
@@ -572,8 +572,8 @@
   ShouldExtI32Return(TLI.ShouldExtI32Return),
   ShouldSignExtI32Param(TLI.ShouldSignExtI32Param) {
   memcpy(AvailableArray, TLI.AvailableArray, sizeof(AvailableArray));
-  for (unsigned i = 0; i < NumVecLibs; i++)
-VecLibDescs[i] = TLI.VecLibDescs[i];
+  VectorDescs = TLI.VectorDescs;
+  ScalarDescs = TLI.ScalarDescs;
 }
 
 TargetLibraryInfoImpl::TargetLibraryInfoImpl(TargetLibraryInfoImpl &&TLI)
@@ -583,8 +583,8 @@
   ShouldSignExtI32Param(TLI.ShouldSignExtI32Param) {
   std::move(std::begin(TLI.AvailableArray), std::end(TLI.AvailableArray),
 AvailableArray);
-  for (unsigned i = 0; i < NumVecLibs; i++)
-VecLibDescs[i] = TLI.VecLibDescs[i];
+  VectorDescs = TLI.VectorDescs;
+  ScalarDescs = TLI.ScalarDescs;
 }
 
 TargetLibraryInfoImpl &TargetLibraryInfoImpl::operator=(const TargetLibraryInfoImpl &TLI) {
@@ -1520,16 +1520,7 @@
   return LHS.VectorFnName < S;
 }
 
-void TargetLibraryInfoImpl::addAllVectorizableFunctions() {
-  addVectorizableFunctionsFromVecLib(Accelerate, VecLibDescs[Accelerate]);
-  addVectorizableFunctionsFromVecLib(MASSV, VecLibDescs[MASSV]);
-  addVectorizableFunctionsFromVecLib(SVML, VecLibDescs[SVML]);
-}
-
-void TargetLibraryInfoImpl::addVectorizableFunct

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 regression testing.


Yes I pinged some of the XLA folks to make it happen.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77632/new/

https://reviews.llvm.org/D77632



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 not have much C++ unittests in general, so most of 
the "features" like this one that LLVM offers as a library are just an artifact 
of being "designed as a library" and being mindful about the layering.
From this point of view this patch is changing the design of a component that 
was modular/pluggable into a closed system. I'm perfectly fine with trying to 
add a unit-test, I just don't know yet where it would fit in the LLVM testing 
though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77632/new/

https://reviews.llvm.org/D77632



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81223/new/

https://reviews.llvm.org/D81223



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 needed to standardize the use of 
> OptimizationLevel across LLVM.


The distinction of size level for LTO isn't obvious to me, this is why I'm 
asking some clarification here.
In general with LTO the Os/Oz frontend flags will trigger the addition of 
function attributes, however the Os/Oz flags aren't impacting the optimizer 
pipeline during LTO (so they basically have no effect during LTO and get mapped 
to O2  directly).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81223/new/

https://reviews.llvm.org/D81223



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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://reviews.llvm.org/D81422

Files:
  clang/test/CodeGenObjC/externally-retained.m
  clang/test/Driver/rocm-device-libs.cl
  compiler-rt/test/fuzzer/fork.test
  debuginfo-tests/llvm-prettyprinters/gdb/llvm-support.gdb
  llvm/docs/CommandGuide/FileCheck.rst
  llvm/test/CodeGen/AArch64/speculation-hardening-dagisel.ll
  llvm/test/CodeGen/AArch64/speculation-hardening-loads.ll
  llvm/test/CodeGen/AArch64/speculation-hardening.ll
  llvm/test/CodeGen/AArch64/speculation-hardening.mir
  llvm/test/FileCheck/comment/after-words.txt
  llvm/test/FileCheck/comment/blank-comments.txt
  llvm/test/FileCheck/comment/suffixes.txt
  llvm/test/FileCheck/comment/suppresses-checks.txt
  llvm/test/FileCheck/comment/unused-comment-prefixes.txt
  llvm/test/FileCheck/dump-input-enable.txt
  llvm/test/FileCheck/envvar-opts.txt
  llvm/test/FileCheck/lit.local.cfg
  llvm/test/FileCheck/match-full-lines.txt
  llvm/test/FileCheck/verbose.txt
  llvm/test/Transforms/InstCombine/fortify-folding.ll
  llvm/utils/FileCheck/FileCheck.cpp
  llvm/utils/lit/lit/TestingConfig.py
  llvm/utils/lit/tests/lit.cfg
  mlir/test/Analysis/test-callgraph.mlir
  mlir/test/Analysis/test-dominance.mlir
  mlir/test/Analysis/test-liveness.mlir
  mlir/test/Conversion/GPUToNVVM/gpu-to-nvvm.mlir
  mlir/test/Conversion/GPUToROCDL/gpu-to-rocdl.mlir
  mlir/test/Conversion/SCFToGPU/no_blocks_no_threads.mlir
  mlir/test/Conversion/SCFToGPU/parallel_loop.mlir
  mlir/test/Conversion/ShapeToStandard/shape-to-standard.mlir
  mlir/test/Dialect/GPU/outlining.mlir
  mlir/test/Dialect/Linalg/fusion-tensor.mlir
  mlir/test/Dialect/Linalg/fusion.mlir
  mlir/test/Dialect/Linalg/fusion_indexed_generic.mlir
  mlir/test/Dialect/Linalg/parallel_loops.mlir
  mlir/test/Dialect/Linalg/tensors-to-buffers.mlir
  mlir/test/Dialect/Linalg/tile_conv_padding.mlir
  mlir/test/Dialect/Linalg/tile_parallel.mlir
  mlir/test/Dialect/SCF/ops.mlir
  mlir/test/Dialect/SCF/parallel-loop-fusion.mlir
  mlir/test/Dialect/SCF/parallel-loop-specialization.mlir
  mlir/test/Dialect/SCF/parallel-loop-tiling.mlir
  mlir/test/Dialect/Shape/ops.mlir
  mlir/test/Dialect/Shape/shape-to-shape.mlir
  mlir/test/Dialect/Standard/expand-atomic.mlir
  mlir/test/Dialect/Vector/vector-contract-transforms.mlir
  mlir/test/Dialect/Vector/vector-flat-transforms.mlir
  mlir/test/EDSC/builder-api-test.cpp
  mlir/test/IR/print-op-local-scope.mlir
  mlir/test/Transforms/buffer-placement-preparation-allowed-memref-results.mlir
  mlir/test/Transforms/buffer-placement-preparation.mlir
  mlir/test/Transforms/buffer-placement.mlir
  mlir/test/Transforms/canonicalize.mlir
  mlir/test/Transforms/sccp-callgraph.mlir
  mlir/test/mlir-tblgen/op-attribute.td
  mlir/test/mlir-tblgen/op-decl.td
  mlir/test/mlir-tblgen/op-derived-attribute.mlir
  mlir/test/mlir-tblgen/op-format-spec.td
  mlir/test/mlir-tblgen/op-interface.td
  mlir/test/mlir-tblgen/pattern.mlir
  mlir/test/mlir-tblgen/predicate.td
  mlir/test/mlir-tblgen/return-types.mlir

Index: mlir/test/mlir-tblgen/return-types.mlir
===
--- mlir/test/mlir-tblgen/return-types.mlir
+++ mlir/test/mlir-tblgen/return-types.mlir
@@ -1,4 +1,4 @@
-// RUN: mlir-opt %s -test-return-type -split-input-file -verify-diagnostics | FileCheck %s --dump-input-on-failure
+// RUN: mlir-opt %s -test-return-type -split-input-file -verify-diagnostics | FileCheck %s
 
 // CHECK-LABEL: testCreateFunctions
 // This function tests invoking the create method with different inference
Index: mlir/test/mlir-tblgen/predicate.td
===
--- mlir/test/mlir-tblgen/predicate.td
+++ mlir/test/mlir-tblgen/predicate.td
@@ -1,4 +1,4 @@
-// RUN: mlir-tblgen -gen-op-defs -I %S/../../include %s | FileCheck %s --dump-input-on-failure
+// RUN: mlir-tblgen -gen-op-defs -I %S/../../include %s | FileCheck %s
 
 include "mlir/IR/OpBase.td"
 
Index: mlir/test/mlir-tblgen/pattern.mlir
===
--- mlir/test/mlir-tblgen/pattern.mlir
+++ mlir/test/mlir-tblgen/pattern.mlir
@@ -1,4 +1,4 @@
-// RUN: mlir-opt -test-patterns -mlir-print-debuginfo %s | FileCheck %s --dump-input-on-failure
+// RUN: mlir-opt -test-patterns -mlir-print-debuginfo %s | FileCheck %s
 
 // CHECK-LABEL: verifyFusedLocs
 func @verifyFusedLocs(%arg0 : i32) -> i32 {
Index: mlir/test/mlir-tblgen/op-interface.td
===
--- mlir/test/mlir-tblgen/op-interface.td
+++ mlir/test/mlir-tblgen/op-interface.td
@@ -1,5 +1,5 @@
-// RUN: mlir-tblgen -gen-op-interface-decls -I %S/../../include %s | FileCheck %s --check-prefix=DECL --dump-input-on-failure
-// RUN: m

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

[Mehdi AMINI via Phabricator via cfe-commits]

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, shauheen, 
jpienaar, rriddle, jfb, arphaman, jholewinski.
Herald added a reviewer: nicolasvasilache.
Herald added a reviewer: herhut.
Herald added a reviewer: aartbik.
Herald added a reviewer: silvas.
Herald added projects: clang, Sanitizers, MLIR.

Update tests


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81422/new/

https://reviews.llvm.org/D81422

Files:
  clang/test/CodeGenObjC/externally-retained.m
  clang/test/Driver/rocm-device-libs.cl
  compiler-rt/test/fuzzer/fork.test
  debuginfo-tests/llvm-prettyprinters/gdb/llvm-support.gdb
  llvm/docs/CommandGuide/FileCheck.rst
  llvm/test/CodeGen/AArch64/speculation-hardening-dagisel.ll
  llvm/test/CodeGen/AArch64/speculation-hardening-loads.ll
  llvm/test/CodeGen/AArch64/speculation-hardening.ll
  llvm/test/CodeGen/AArch64/speculation-hardening.mir
  llvm/test/FileCheck/dump-input-enable.txt
  llvm/test/Transforms/InstCombine/fortify-folding.ll
  llvm/utils/FileCheck/FileCheck.cpp
  mlir/test/Analysis/test-callgraph.mlir
  mlir/test/Analysis/test-dominance.mlir
  mlir/test/Analysis/test-liveness.mlir
  mlir/test/Conversion/GPUToNVVM/gpu-to-nvvm.mlir
  mlir/test/Conversion/GPUToROCDL/gpu-to-rocdl.mlir
  mlir/test/Conversion/SCFToGPU/no_blocks_no_threads.mlir
  mlir/test/Conversion/SCFToGPU/parallel_loop.mlir
  mlir/test/Conversion/ShapeToStandard/shape-to-standard.mlir
  mlir/test/Dialect/GPU/outlining.mlir
  mlir/test/Dialect/Linalg/fusion-tensor.mlir
  mlir/test/Dialect/Linalg/fusion.mlir
  mlir/test/Dialect/Linalg/fusion_indexed_generic.mlir
  mlir/test/Dialect/Linalg/parallel_loops.mlir
  mlir/test/Dialect/Linalg/tensors-to-buffers.mlir
  mlir/test/Dialect/Linalg/tile_conv_padding.mlir
  mlir/test/Dialect/Linalg/tile_parallel.mlir
  mlir/test/Dialect/SCF/ops.mlir
  mlir/test/Dialect/SCF/parallel-loop-fusion.mlir
  mlir/test/Dialect/SCF/parallel-loop-specialization.mlir
  mlir/test/Dialect/SCF/parallel-loop-tiling.mlir
  mlir/test/Dialect/Shape/ops.mlir
  mlir/test/Dialect/Shape/shape-to-shape.mlir
  mlir/test/Dialect/Standard/expand-atomic.mlir
  mlir/test/Dialect/Vector/vector-contract-transforms.mlir
  mlir/test/Dialect/Vector/vector-flat-transforms.mlir
  mlir/test/EDSC/builder-api-test.cpp
  mlir/test/IR/print-op-local-scope.mlir
  mlir/test/Transforms/buffer-placement-preparation-allowed-memref-results.mlir
  mlir/test/Transforms/buffer-placement-preparation.mlir
  mlir/test/Transforms/buffer-placement.mlir
  mlir/test/Transforms/canonicalize.mlir
  mlir/test/Transforms/sccp-callgraph.mlir
  mlir/test/mlir-tblgen/op-attribute.td
  mlir/test/mlir-tblgen/op-decl.td
  mlir/test/mlir-tblgen/op-derived-attribute.mlir
  mlir/test/mlir-tblgen/op-format-spec.td
  mlir/test/mlir-tblgen/op-interface.td
  mlir/test/mlir-tblgen/pattern.mlir
  mlir/test/mlir-tblgen/predicate.td
  mlir/test/mlir-tblgen/return-types.mlir

Index: mlir/test/mlir-tblgen/return-types.mlir
===
--- mlir/test/mlir-tblgen/return-types.mlir
+++ mlir/test/mlir-tblgen/return-types.mlir
@@ -1,4 +1,4 @@
-// RUN: mlir-opt %s -test-return-type -split-input-file -verify-diagnostics | FileCheck %s --dump-input-on-failure
+// RUN: mlir-opt %s -test-return-type -split-input-file -verify-diagnostics | FileCheck %s
 
 // CHECK-LABEL: testCreateFunctions
 // This function tests invoking the create method with different inference
Index: mlir/test/mlir-tblgen/predicate.td
===
--- mlir/test/mlir-tblgen/predicate.td
+++ mlir/test/mlir-tblgen/predicate.td
@@ -1,4 +1,4 @@
-// RUN: mlir-tblgen -gen-op-defs -I %S/../../include %s | FileCheck %s --dump-input-on-failure
+// RUN: mlir-tblgen -gen-op-defs -I %S/../../include %s | FileCheck %s
 
 include "mlir/IR/OpBase.td"
 
Index: mlir/test/mlir-tblgen/pattern.mlir
===
--- mlir/test/mlir-tblgen/pattern.mlir
+++ mlir/test/mlir-tblgen/pattern.mlir
@@ -1,4 +1,4 @@
-// RUN: mlir-opt -test-patterns -mlir-print-debuginfo %s | FileCheck %s --dump-input-on-failure
+// RUN: mlir-opt -test-patterns -mlir-print-debuginfo %s | FileCheck %s
 
 // CHECK-LABEL: verifyFusedLocs
 func @verifyFusedLocs(%arg0 : i32) -> i32 {
Index: mlir/test/mlir-tblgen/op-interface.td
===
--- mlir/test/mlir-tblgen/op-interface.td
+++ mlir/test/mlir-tblgen/op-interface.td
@@ -1,5 +1,5 @@
-// RUN: mlir-tblgen -gen-op-interface-decls -I %S/../../include %s | FileCheck %s --check-prefix=DECL --dump-input-on-failure
-// RUN: mlir-tblgen -gen-op-decls -I %S/../../include %s | FileCheck %s --check-prefix=OP_DE

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 including the input on 
failure could indeed help debugging when something fails in flaky way, or is 
dependent on the environment (like the path where the code is checked out, 
etc.). 
At least in the past it could have saved me trouble when only a single bot was 
failing in mysterious way and the bot owner had to be involved to help figure 
out.

(also right now we're inconsistent: many tests are adding the flag explicitly)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81422/new/

https://reviews.llvm.org/D81422



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 that the default is fail?
Now they do :)
I originally pushed to Phab to get the pre-merge testing running while I was 
still fixing a few things locally, I didn't expect such fast reviews!
Should be all good now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81422/new/

https://reviews.llvm.org/D81422



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81422/new/

https://reviews.llvm.org/D81422

Files:
  clang/test/CodeGenObjC/externally-retained.m
  clang/test/Driver/rocm-device-libs.cl
  compiler-rt/test/fuzzer/fork.test
  debuginfo-tests/llvm-prettyprinters/gdb/llvm-support.gdb
  llvm/docs/CommandGuide/FileCheck.rst
  llvm/test/CodeGen/AArch64/speculation-hardening-dagisel.ll
  llvm/test/CodeGen/AArch64/speculation-hardening-loads.ll
  llvm/test/CodeGen/AArch64/speculation-hardening.ll
  llvm/test/CodeGen/AArch64/speculation-hardening.mir
  llvm/test/FileCheck/comment/after-words.txt
  llvm/test/FileCheck/comment/blank-comments.txt
  llvm/test/FileCheck/comment/suffixes.txt
  llvm/test/FileCheck/comment/suppresses-checks.txt
  llvm/test/FileCheck/comment/unused-comment-prefixes.txt
  llvm/test/FileCheck/dump-input-enable.txt
  llvm/test/FileCheck/envvar-opts.txt
  llvm/test/FileCheck/lit.local.cfg
  llvm/test/FileCheck/match-full-lines.txt
  llvm/test/FileCheck/verbose.txt
  llvm/test/Transforms/InstCombine/fortify-folding.ll
  llvm/utils/FileCheck/FileCheck.cpp
  llvm/utils/lit/lit/TestingConfig.py
  llvm/utils/lit/tests/lit.cfg
  mlir/test/Analysis/test-callgraph.mlir
  mlir/test/Analysis/test-dominance.mlir
  mlir/test/Analysis/test-liveness.mlir
  mlir/test/Conversion/GPUToNVVM/gpu-to-nvvm.mlir
  mlir/test/Conversion/GPUToROCDL/gpu-to-rocdl.mlir
  mlir/test/Conversion/SCFToGPU/no_blocks_no_threads.mlir
  mlir/test/Conversion/SCFToGPU/parallel_loop.mlir
  mlir/test/Conversion/ShapeToStandard/shape-to-standard.mlir
  mlir/test/Dialect/GPU/outlining.mlir
  mlir/test/Dialect/Linalg/fusion-tensor.mlir
  mlir/test/Dialect/Linalg/fusion.mlir
  mlir/test/Dialect/Linalg/fusion_indexed_generic.mlir
  mlir/test/Dialect/Linalg/parallel_loops.mlir
  mlir/test/Dialect/Linalg/tensors-to-buffers.mlir
  mlir/test/Dialect/Linalg/tile_conv_padding.mlir
  mlir/test/Dialect/Linalg/tile_parallel.mlir
  mlir/test/Dialect/SCF/ops.mlir
  mlir/test/Dialect/SCF/parallel-loop-fusion.mlir
  mlir/test/Dialect/SCF/parallel-loop-specialization.mlir
  mlir/test/Dialect/SCF/parallel-loop-tiling.mlir
  mlir/test/Dialect/Shape/ops.mlir
  mlir/test/Dialect/Shape/shape-to-shape.mlir
  mlir/test/Dialect/Standard/expand-atomic.mlir
  mlir/test/Dialect/Vector/vector-contract-transforms.mlir
  mlir/test/Dialect/Vector/vector-flat-transforms.mlir
  mlir/test/EDSC/builder-api-test.cpp
  mlir/test/IR/print-op-local-scope.mlir
  mlir/test/Transforms/buffer-placement-preparation-allowed-memref-results.mlir
  mlir/test/Transforms/buffer-placement-preparation.mlir
  mlir/test/Transforms/buffer-placement.mlir
  mlir/test/Transforms/canonicalize.mlir
  mlir/test/Transforms/sccp-callgraph.mlir
  mlir/test/mlir-tblgen/op-attribute.td
  mlir/test/mlir-tblgen/op-decl.td
  mlir/test/mlir-tblgen/op-derived-attribute.mlir
  mlir/test/mlir-tblgen/op-format-spec.td
  mlir/test/mlir-tblgen/op-interface.td
  mlir/test/mlir-tblgen/pattern.mlir
  mlir/test/mlir-tblgen/predicate.td
  mlir/test/mlir-tblgen/return-types.mlir

Index: mlir/test/mlir-tblgen/return-types.mlir
===
--- mlir/test/mlir-tblgen/return-types.mlir
+++ mlir/test/mlir-tblgen/return-types.mlir
@@ -1,4 +1,4 @@
-// RUN: mlir-opt %s -test-return-type -split-input-file -verify-diagnostics | FileCheck %s --dump-input-on-failure
+// RUN: mlir-opt %s -test-return-type -split-input-file -verify-diagnostics | FileCheck %s
 
 // CHECK-LABEL: testCreateFunctions
 // This function tests invoking the create method with different inference
Index: mlir/test/mlir-tblgen/predicate.td
===
--- mlir/test/mlir-tblgen/predicate.td
+++ mlir/test/mlir-tblgen/predicate.td
@@ -1,4 +1,4 @@
-// RUN: mlir-tblgen -gen-op-defs -I %S/../../include %s | FileCheck %s --dump-input-on-failure
+// RUN: mlir-tblgen -gen-op-defs -I %S/../../include %s | FileCheck %s
 
 include "mlir/IR/OpBase.td"
 
Index: mlir/test/mlir-tblgen/pattern.mlir
===
--- mlir/test/mlir-tblgen/pattern.mlir
+++ mlir/test/mlir-tblgen/pattern.mlir
@@ -1,4 +1,4 @@
-// RUN: mlir-opt -test-patterns -mlir-print-debuginfo %s | FileCheck %s --dump-input-on-failure
+// RUN: mlir-opt -test-patterns -mlir-print-debuginfo %s | FileCheck %s
 
 // CHECK-LABEL: verifyFusedLocs
 func @verifyFusedLocs(%arg0 : i32) -> i32 {
Index: mlir/test/mlir-tblgen/op-interface.td
===
--- mlir/test/mlir-tblgen/op-interface.td
+++ mlir/test/mlir-tblgen/op-interface.td
@@ -1,5 +1,5 @@
-// RUN: mlir-tblgen -gen-op-interfac

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 `-dump-input=never` to this first FileCheck call.  Otherwise, the 
> example is broken because FileCheck never prints `expected string found in 
> input` when `-dump-input=fail` regardless of `-v`.
Well spotted!



Comment at: llvm/utils/FileCheck/FileCheck.cpp:117
- "FILECHECK_DUMP_INPUT_ON_FAILURE environment variable.\n"
- "This option is deprecated in favor of -dump-input=fail.\n"));
 

jdenny wrote:
> jdenny wrote:
> > Please mention in the patch summary that `-dump-input-on-failure` and 
> > `FILECHECK_DUMP_INPUT_ON_FAILURE` are being dropped.
> This is marked as done, but I don't see the change.
That's because I amended my commit locally but `arc` does not propagate the 
update to Phabricator apparently: https://phabricator.kde.org/T7711 ; I'll do 
it manually


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81422/new/

https://reviews.llvm.org/D81422



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81422/new/

https://reviews.llvm.org/D81422

Files:
  clang/test/CodeGenObjC/externally-retained.m
  clang/test/Driver/rocm-device-libs.cl
  compiler-rt/test/fuzzer/fork.test
  debuginfo-tests/llvm-prettyprinters/gdb/llvm-support.gdb
  llvm/docs/CommandGuide/FileCheck.rst
  llvm/test/CodeGen/AArch64/speculation-hardening-dagisel.ll
  llvm/test/CodeGen/AArch64/speculation-hardening-loads.ll
  llvm/test/CodeGen/AArch64/speculation-hardening.ll
  llvm/test/CodeGen/AArch64/speculation-hardening.mir
  llvm/test/FileCheck/comment/after-words.txt
  llvm/test/FileCheck/comment/blank-comments.txt
  llvm/test/FileCheck/comment/suffixes.txt
  llvm/test/FileCheck/comment/suppresses-checks.txt
  llvm/test/FileCheck/comment/unused-comment-prefixes.txt
  llvm/test/FileCheck/dump-input-enable.txt
  llvm/test/FileCheck/envvar-opts.txt
  llvm/test/FileCheck/lit.local.cfg
  llvm/test/FileCheck/match-full-lines.txt
  llvm/test/FileCheck/verbose.txt
  llvm/test/Transforms/InstCombine/fortify-folding.ll
  llvm/utils/FileCheck/FileCheck.cpp
  llvm/utils/lit/lit/TestingConfig.py
  llvm/utils/lit/tests/lit.cfg
  mlir/test/Analysis/test-callgraph.mlir
  mlir/test/Analysis/test-dominance.mlir
  mlir/test/Analysis/test-liveness.mlir
  mlir/test/Conversion/GPUToNVVM/gpu-to-nvvm.mlir
  mlir/test/Conversion/GPUToROCDL/gpu-to-rocdl.mlir
  mlir/test/Conversion/SCFToGPU/no_blocks_no_threads.mlir
  mlir/test/Conversion/SCFToGPU/parallel_loop.mlir
  mlir/test/Conversion/ShapeToStandard/shape-to-standard.mlir
  mlir/test/Dialect/GPU/outlining.mlir
  mlir/test/Dialect/Linalg/fusion-tensor.mlir
  mlir/test/Dialect/Linalg/fusion.mlir
  mlir/test/Dialect/Linalg/fusion_indexed_generic.mlir
  mlir/test/Dialect/Linalg/parallel_loops.mlir
  mlir/test/Dialect/Linalg/tensors-to-buffers.mlir
  mlir/test/Dialect/Linalg/tile_conv_padding.mlir
  mlir/test/Dialect/Linalg/tile_parallel.mlir
  mlir/test/Dialect/SCF/ops.mlir
  mlir/test/Dialect/SCF/parallel-loop-fusion.mlir
  mlir/test/Dialect/SCF/parallel-loop-specialization.mlir
  mlir/test/Dialect/SCF/parallel-loop-tiling.mlir
  mlir/test/Dialect/Shape/ops.mlir
  mlir/test/Dialect/Shape/shape-to-shape.mlir
  mlir/test/Dialect/Standard/expand-atomic.mlir
  mlir/test/Dialect/Vector/vector-contract-transforms.mlir
  mlir/test/Dialect/Vector/vector-flat-transforms.mlir
  mlir/test/EDSC/builder-api-test.cpp
  mlir/test/IR/print-op-local-scope.mlir
  mlir/test/Transforms/buffer-placement-preparation-allowed-memref-results.mlir
  mlir/test/Transforms/buffer-placement-preparation.mlir
  mlir/test/Transforms/buffer-placement.mlir
  mlir/test/Transforms/canonicalize.mlir
  mlir/test/Transforms/sccp-callgraph.mlir
  mlir/test/mlir-tblgen/op-attribute.td
  mlir/test/mlir-tblgen/op-decl.td
  mlir/test/mlir-tblgen/op-derived-attribute.mlir
  mlir/test/mlir-tblgen/op-format-spec.td
  mlir/test/mlir-tblgen/op-interface.td
  mlir/test/mlir-tblgen/pattern.mlir
  mlir/test/mlir-tblgen/predicate.td
  mlir/test/mlir-tblgen/return-types.mlir

Index: mlir/test/mlir-tblgen/return-types.mlir
===
--- mlir/test/mlir-tblgen/return-types.mlir
+++ mlir/test/mlir-tblgen/return-types.mlir
@@ -1,4 +1,4 @@
-// RUN: mlir-opt %s -test-return-type -split-input-file -verify-diagnostics | FileCheck %s --dump-input-on-failure
+// RUN: mlir-opt %s -test-return-type -split-input-file -verify-diagnostics | FileCheck %s
 
 // CHECK-LABEL: testCreateFunctions
 // This function tests invoking the create method with different inference
Index: mlir/test/mlir-tblgen/predicate.td
===
--- mlir/test/mlir-tblgen/predicate.td
+++ mlir/test/mlir-tblgen/predicate.td
@@ -1,4 +1,4 @@
-// RUN: mlir-tblgen -gen-op-defs -I %S/../../include %s | FileCheck %s --dump-input-on-failure
+// RUN: mlir-tblgen -gen-op-defs -I %S/../../include %s | FileCheck %s
 
 include "mlir/IR/OpBase.td"
 
Index: mlir/test/mlir-tblgen/pattern.mlir
===
--- mlir/test/mlir-tblgen/pattern.mlir
+++ mlir/test/mlir-tblgen/pattern.mlir
@@ -1,4 +1,4 @@
-// RUN: mlir-opt -test-patterns -mlir-print-debuginfo %s | FileCheck %s --dump-input-on-failure
+// RUN: mlir-opt -test-patterns -mlir-print-debuginfo %s | FileCheck %s
 
 // CHECK-LABEL: verifyFusedLocs
 func @verifyFusedLocs(%arg0 : i32) -> i32 {
Index: mlir/test/mlir-tblgen/op-interface.td
===
--- mlir/test/mlir-tblgen/op-interface.td
+++ mlir/test/mlir-tblgen/op-interface.td
@@ -1,5 +1,5 @@
-// RUN: mlir-tblgen -gen-op-interface-decls -I 

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

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

In D81422#2083882 , @arsenm wrote:

> In D81422#2083808 , @mehdi_amini 
> wrote:
>
> > 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?
>
>
> This isn't really manageable, especially with the trend of using update_* 
> test checks scripts. Stuff like legalization tests just have to stress every 
> combination of inputs.


This is something that the script generating every combination could manage to 
split as well?
Alternatively, can these very large test be appended `--dump-input=never` on 
the RUN line? (maybe the test generator can do this?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81422/new/

https://reviews.llvm.org/D81422



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 my desk and reading a 
test output to understand the failure: this goes beyond build bots.

(you have the environment variable you can set locally if you don't want the 
behavior though)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81422/new/

https://reviews.llvm.org/D81422



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 with any patch: this 
isn't how LTO is setup today.

>   For example, we could have a different tuning for inlining,  vectorization, 
> etc. 

All these are covered by the function attributes already.

> We could also use the size level to automatically enable optimizations such 
> as HotColdSplitting, MergeFunctions, etc., instead of relying on specific 
> enabling flags. We could also have other size-specific optimizations in the 
> future, such as MergeSimilarFunctions (https://reviews.llvm.org/D52896).

All these could be in the LTO pipeline and driven by the attribute as well.

> I believe that function attributes for size are useful for optimizing cold 
> functions that have been outlined by HotColdSplitting, for example.

The attribute is added by the frontend and can change per translation-unit / 
per function though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81223/new/

https://reviews.llvm.org/D81223



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81422/new/

https://reviews.llvm.org/D81422



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 current CHECK-LABEL section only: it seems like it 
> > could reduce the output drastically while still preserving how useful the 
> > information is?
>
>
> One FileCheck invocation can report multiple errors, one per CHECK-LABEL 
> section, and I prefer to see all errors every time.


Yeah I had in mind to show all the errors for the current section, then the 
input for the section, before moving to the next section.

> I also prefer to see the entire input dump (with annotations produced by -vv) 
> as sometimes the error FileCheck reports is far away from the actual error.  
> For example, maybe the CHECK-LABEL directives matched text at unexpected 
> locations in the input, and then the directives in their sections failed as a 
> result. It might be hard to determine that the CHECK-LABEL directives were at 
> fault without seeing the text it should have matched outside of its 
> (incorrect) section.

Sure: but that seems like not the most common case to me: if we start by 
showing the section we matched it provides already a pretty good indication.

> If you choose to limit the dump to one CHECK-LABEL section, please make that 
> behavior optional via a command-line option (which can be set via 
> FILECHECK_OPTS).

I thought it would be a better default actually, leaving the full-full as opt-in


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81422/new/

https://reviews.llvm.org/D81422



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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.org/D81045/new/

https://reviews.llvm.org/D81045

Files:
  clang/include/clang/AST/DeclBase.h
  llvm/include/llvm/Support/Casting.h
  llvm/lib/Analysis/ScalarEvolution.cpp
  llvm/lib/Target/PowerPC/PPCBoolRetToInt.cpp


Index: llvm/lib/Target/PowerPC/PPCBoolRetToInt.cpp
===
--- llvm/lib/Target/PowerPC/PPCBoolRetToInt.cpp
+++ llvm/lib/Target/PowerPC/PPCBoolRetToInt.cpp
@@ -220,7 +220,7 @@
 auto Defs = findAllDefs(U);
 
 // If the values are all Constants or Arguments, don't bother
-if (llvm::none_of(Defs, isa))
+if (llvm::none_of(Defs, [](Value *V) { return isa(V); }))
   return false;
 
 // Presently, we only know how to handle PHINode, Constant, Arguments and
Index: llvm/lib/Analysis/ScalarEvolution.cpp
===
--- llvm/lib/Analysis/ScalarEvolution.cpp
+++ llvm/lib/Analysis/ScalarEvolution.cpp
@@ -3860,7 +3860,8 @@
   if (I != HasRecMap.end())
 return I->second;
 
-  bool FoundAddRec = SCEVExprContains(S, isa);
+  bool FoundAddRec =
+  SCEVExprContains(S, [](const SCEV *S) { return isa(S); 
});
   HasRecMap.insert({S, FoundAddRec});
   return FoundAddRec;
 }
@@ -11201,8 +11202,9 @@
 // Returns true when one of the SCEVs of Terms contains a SCEVUnknown 
parameter.
 static inline bool containsParameters(SmallVectorImpl &Terms) {
   for (const SCEV *T : Terms)
-if (SCEVExprContains(T, isa))
+if (SCEVExprContains(T, [](const SCEV *S) { return isa(S); }))
   return true;
+
   return false;
 }
 
Index: llvm/include/llvm/Support/Casting.h
===
--- llvm/include/llvm/Support/Casting.h
+++ llvm/include/llvm/Support/Casting.h
@@ -132,24 +132,30 @@
   }
 };
 
-// isa - Return true if the parameter to the template is an instance of the
-// template type argument.  Used like this:
+// isa - Return true if the parameter to the template is an instance of one
+// of the template type arguments.  Used like this:
 //
 //  if (isa(myVal)) { ... }
+//  if (isa(myVal)) { ... }
 //
 template  LLVM_NODISCARD inline bool isa(const Y &Val) {
   return isa_impl_wrap::SimpleType>::doit(Val);
 }
 
+template 
+LLVM_NODISCARD inline bool isa(const Y &Val) {
+  return isa(Val) || isa(Val);
+}
+
 // isa_and_nonnull - Functionally identical to isa, except that a null value
 // is accepted.
 //
-template 
+template 
 LLVM_NODISCARD inline bool isa_and_nonnull(const Y &Val) {
   if (!Val)
 return false;
-  return isa(Val);
+  return isa(Val);
 }
 
 
//===--===//
Index: clang/include/clang/AST/DeclBase.h
===
--- clang/include/clang/AST/DeclBase.h
+++ clang/include/clang/AST/DeclBase.h
@@ -518,7 +518,7 @@
 if (!HasAttrs) return;
 
 AttrVec &Vec = getAttrs();
-Vec.erase(std::remove_if(Vec.begin(), Vec.end(), isa), 
Vec.end());
+llvm::erase_if(Vec, [](Attr *A) { return isa(A); });
 
 if (Vec.empty())
   HasAttrs = false;


Index: llvm/lib/Target/PowerPC/PPCBoolRetToInt.cpp
===
--- llvm/lib/Target/PowerPC/PPCBoolRetToInt.cpp
+++ llvm/lib/Target/PowerPC/PPCBoolRetToInt.cpp
@@ -220,7 +220,7 @@
 auto Defs = findAllDefs(U);
 
 // If the values are all Constants or Arguments, don't bother
-if (llvm::none_of(Defs, isa))
+if (llvm::none_of(Defs, [](Value *V) { return isa(V); }))
   return false;
 
 // Presently, we only know how to handle PHINode, Constant, Arguments and
Index: llvm/lib/Analysis/ScalarEvolution.cpp
===
--- llvm/lib/Analysis/ScalarEvolution.cpp
+++ llvm/lib/Analysis/ScalarEvolution.cpp
@@ -3860,7 +3860,8 @@
   if (I != HasRecMap.end())
 return I->second;
 
-  bool FoundAddRec = SCEVExprContains(S, isa);
+  bool FoundAddRec =
+  SCEVExprContains(S, [](const SCEV *S) { return isa(S); });
   HasRecMap.insert({S, FoundAddRec});
   return FoundAddRec;
 }
@@ -11201,8 +11202,9 @@
 // Returns true when one of the SCEVs of Terms contains a SCEVUnknown parameter.
 static inline bool containsParameters(SmallVectorImpl &Terms) {
   for (const SCEV *T : Terms)
-if (SCEVExprContains(T, isa))
+if (SCEVExprContains(T, [](const SCEV *S) { return isa(S); }))
   return true;
+
   return false;
 }
 
Index: llvm/include/llvm/Support/Casting.h
===
--- llvm/include/llvm/Support/Casting.h
+++ llvm/include/llvm/Support/Casting.h
@@ -132,24 +132,30 @@
   

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

[Mehdi AMINI via Phabricator via cfe-commits]

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


I have the totally opposite experience: I can't do any debugging efficiently 
without this option.  Empirically, most new-comers to writing tests with 
FileCheck (in MLIR, TensorFlow, XLA, IREE, etc.) have been manually adding 
-dump-input-on-failure hard-coded into the test themselves, which is a pretty 
good indication.

Having something contextual around the failures would work for me as well: but 
the previous situation was just not usable for me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81422/new/

https://reviews.llvm.org/D81422



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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


Would you like a lit command line flag that set the environment variable? I 
think that's easily doable




Comment at: mlir/test/mlir-tblgen/op-format-spec.td:1
-// RUN: mlir-tblgen -gen-op-decls -asmformat-error-is-fatal=false -I 
%S/../../include %s -o=%t 2>&1 | FileCheck %s --dump-input-on-failure
+// RUN: mlir-tblgen -gen-op-decls -asmformat-error-is-fatal=false -I 
%S/../../include %s -o=%t 2>&1 | FileCheck %s
 

arsenm wrote:
> All of these MLIR tests are microscopic and I don't think this is a 
> representative sample across all the projects. Most testcases are 
> significantly larger, and have hundreds if not thousands of lines of output
Well, if this can act as an extra incentive to break-up such large tests, it 
see it as a win: debugging failing patterns in the middle of such a large test 
output is not nice regardless.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81422/new/

https://reviews.llvm.org/D81422



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65761/new/

https://reviews.llvm.org/D65761



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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
  https://reviews.llvm.org/D124750/new/

https://reviews.llvm.org/D124750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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** C++17 without writing it 
in the coding standards?
I'm not sure it'll be manageable: how do you see this playing out?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130689/new/

https://reviews.llvm.org/D130689

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 standards?
>> I'm not sure it'll be manageable: how do you see this playing out?
>
> Probably poorly worded - what I was trying to convey is that if we want to 
> use a C++17 feature that's really impactful on the syntax/readability we 
> should probably consider recommending ways to use it in the coding standards, 
> similar to our guidelines on using for() loops 
> (https://llvm.org/docs/CodingStandards.html#use-range-based-for-loops-wherever-possible)
>  or the auto keyword 
> (https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable).

Makes sense, thanks! Let's just update the wording to convey this and this all 
looks good to me :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130689/new/

https://reviews.llvm.org/D130689

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 feature, this isn't covered by the "vendor 
extensions" part but by the following paragraph:

> Nevertheless, we restrict ourselves to features which are available in the 
> major toolchains supported as host compilers

This includes not only missing features in libstdc++ but also gcc and clang 
bugs/limitations that we'll have to work around.

> This is also the first time this split becomes relevant AFAIK

I don't : the migration to C++11 was done the same way, piecewise by making 
sure that when we start using a new feature (core or stdlib) it actually works 
on the stated minimum version of the toolchains we support.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130689/new/

https://reviews.llvm.org/D130689

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 ACTION
  https://reviews.llvm.org/D124750/new/

https://reviews.llvm.org/D124750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 that all bots have picked 
up the revision, then revert and survey the results for the runs. Communicating 
ahead of time on this also so that downstream can pickup the revision for their 
own tests if they want.
This should provide a pretty good signal ahead of time of the "real" migration 
hopefully!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130689/new/

https://reviews.llvm.org/D130689

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 always have some post-commit buildbots.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130689/new/

https://reviews.llvm.org/D130689

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 this to a separate change.
Sure, I can commit this separately.



Comment at: llvm/lib/Support/Debug.cpp:96
 //until program termination.
-static cl::opt
-DebugBufferSize("debug-buffer-size",
-cl::desc("Buffer the last N characters of debug output "
- "until program termination. "
- "[default 0 -- immediate print-out]"),
-cl::Hidden,
-cl::init(0));
+struct CreateDebugBufferSize {
+  static void *call() {

jpienaar wrote:
> Why not in anonymous namespace too? (consistenly everywhere)
I think it is already?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105959/new/

https://reviews.llvm.org/D105959

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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:
  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
  clang-tools-extra/clangd/indexer/IndexerMain.cpp
  clang/tools/clang-refactor/ClangRefactor.cpp
  llvm/docs/CommandLine.rst
  llvm/include/llvm/Support/ARMAttributeParser.h
  llvm/include/llvm/Support/ARMBuildAttributes.h
  llvm/include/llvm/Support/CommandLine.h
  llvm/include/llvm/Support/RISCVAttributeParser.h
  llvm/include/llvm/Support/RISCVAttributes.h
  llvm/include/llvm/Support/ScopedPrinter.h
  llvm/include/llvm/Support/WithColor.h
  llvm/lib/Support/ARMBuildAttrs.cpp
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/CommandLine.cpp
  llvm/lib/Support/Debug.cpp
  llvm/lib/Support/DebugCounter.cpp
  llvm/lib/Support/DebugOptions.h
  llvm/lib/Support/ELFAttributeParser.cpp
  llvm/lib/Support/GraphWriter.cpp
  llvm/lib/Support/RISCVAttributes.cpp
  llvm/lib/Support/RandomNumberGenerator.cpp
  llvm/lib/Support/Signals.cpp
  llvm/lib/Support/Statistic.cpp
  llvm/lib/Support/TimeProfiler.cpp
  llvm/lib/Support/Timer.cpp
  llvm/lib/Support/TypeSize.cpp
  llvm/lib/Support/WithColor.cpp
  llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
  llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp
  llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
  llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
  llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
  llvm/unittests/Support/ARMAttributeParser.cpp
  llvm/unittests/Support/CommandLineTest.cpp
  llvm/unittests/Support/RISCVAttributeParserTest.cpp

Index: llvm/unittests/Support/RISCVAttributeParserTest.cpp
===
--- llvm/unittests/Support/RISCVAttributeParserTest.cpp
+++ llvm/unittests/Support/RISCVAttributeParserTest.cpp
@@ -49,7 +49,7 @@
 }
 
 static bool testTagString(unsigned Tag, const char *name) {
-  return ELFAttrs::attrTypeAsString(Tag, RISCVAttrs::RISCVAttributeTags)
+  return ELFAttrs::attrTypeAsString(Tag, RISCVAttrs::getRISCVAttributeTags())
  .str() == name;
 }
 
Index: llvm/unittests/Support/CommandLineTest.cpp
===
--- llvm/unittests/Support/CommandLineTest.cpp
+++ llvm/unittests/Support/CommandLineTest.cpp
@@ -110,7 +110,7 @@
   ASSERT_NE(Retrieved->Categories.end(),
 find_if(Retrieved->Categories,
 [&](const llvm::cl::OptionCategory *Cat) {
-  return Cat == &cl::GeneralCategory;
+  return Cat == &cl::getGeneralCategory();
 }))
   << "Incorrect default option category.";
 
@@ -152,10 +152,10 @@
 
 TEST(CommandLineTest, UseMultipleCategories) {
   StackOption TestOption2("test-option2", cl::cat(TestCategory),
-   cl::cat(cl::GeneralCategory),
-   cl::cat(cl::GeneralCategory));
+   cl::cat(cl::getGeneralCategory()),
+   cl::cat(cl::getGeneralCategory()));
 
-  // Make sure cl::GeneralCategory wasn't added twice.
+  // Make sure cl::getGeneralCategory() wasn't added twice.
   ASSERT_EQ(TestOption2.Categories.size(), 2U);
 
   ASSERT_NE(TestOption2.Categories.end(),
@@ -166,9 +166,9 @@
   << "Failed to assign Option Category.";
   ASSERT_NE(TestOption2.Categories.end(),
 find_if(TestOption2.Categories,
- [&](const llvm::cl::OptionCategory *Cat) {
-   return Cat == &cl::GeneralCategory;
- }))
+[&](const llvm::cl::OptionCategory *Cat) {
+  return Cat == &cl::getGeneralCategory();
+}))
   << "Failed to assign General Category.";
 
   cl::OptionCategory AnotherCategory("Additional test Options", "Description");
@@ -176,9 +176,9 @@
   cl::cat(AnotherCategory));
   ASSERT_EQ(TestOption.Categories.end(),
 find_if(TestOption.Categories,
- [&](const llvm::cl::OptionCategory *Cat) {
-   return Cat == &cl::GeneralCategory;
- }))
+[&](const llvm::cl::OptionCategory *Cat) {
+  return Cat == &cl::getGeneralCategory();
+}))
   << "Failed to remove General Category.";
   ASSERT_NE(TestOption.Categories.end(),
 find_if(TestOption.Categories,
Index: llvm/unittests/Support/ARMAttributeParser.cpp
===
--- llvm/unittests/Support/ARMAttributeParser.cpp
+++ llvm/unit

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

[Mehdi AMINI via Phabricator via cfe-commits]

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
  clang-tools-extra/clangd/indexer/IndexerMain.cpp
  clang/tools/clang-refactor/ClangRefactor.cpp
  llvm/docs/CommandLine.rst
  llvm/include/llvm/Support/ARMAttributeParser.h
  llvm/include/llvm/Support/ARMBuildAttributes.h
  llvm/include/llvm/Support/CommandLine.h
  llvm/include/llvm/Support/RISCVAttributeParser.h
  llvm/include/llvm/Support/RISCVAttributes.h
  llvm/include/llvm/Support/ScopedPrinter.h
  llvm/include/llvm/Support/WithColor.h
  llvm/lib/Support/ARMBuildAttrs.cpp
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/CommandLine.cpp
  llvm/lib/Support/Debug.cpp
  llvm/lib/Support/DebugCounter.cpp
  llvm/lib/Support/DebugOptions.h
  llvm/lib/Support/ELFAttributeParser.cpp
  llvm/lib/Support/GraphWriter.cpp
  llvm/lib/Support/RISCVAttributes.cpp
  llvm/lib/Support/RandomNumberGenerator.cpp
  llvm/lib/Support/Signals.cpp
  llvm/lib/Support/Statistic.cpp
  llvm/lib/Support/TimeProfiler.cpp
  llvm/lib/Support/Timer.cpp
  llvm/lib/Support/TypeSize.cpp
  llvm/lib/Support/Windows/Signals.inc
  llvm/lib/Support/WithColor.cpp
  llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
  llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp
  llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
  llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
  llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
  llvm/unittests/Support/ARMAttributeParser.cpp
  llvm/unittests/Support/CommandLineTest.cpp
  llvm/unittests/Support/RISCVAttributeParserTest.cpp

Index: llvm/unittests/Support/RISCVAttributeParserTest.cpp
===
--- llvm/unittests/Support/RISCVAttributeParserTest.cpp
+++ llvm/unittests/Support/RISCVAttributeParserTest.cpp
@@ -49,7 +49,7 @@
 }
 
 static bool testTagString(unsigned Tag, const char *name) {
-  return ELFAttrs::attrTypeAsString(Tag, RISCVAttrs::RISCVAttributeTags)
+  return ELFAttrs::attrTypeAsString(Tag, RISCVAttrs::getRISCVAttributeTags())
  .str() == name;
 }
 
Index: llvm/unittests/Support/CommandLineTest.cpp
===
--- llvm/unittests/Support/CommandLineTest.cpp
+++ llvm/unittests/Support/CommandLineTest.cpp
@@ -110,7 +110,7 @@
   ASSERT_NE(Retrieved->Categories.end(),
 find_if(Retrieved->Categories,
 [&](const llvm::cl::OptionCategory *Cat) {
-  return Cat == &cl::GeneralCategory;
+  return Cat == &cl::getGeneralCategory();
 }))
   << "Incorrect default option category.";
 
@@ -152,10 +152,10 @@
 
 TEST(CommandLineTest, UseMultipleCategories) {
   StackOption TestOption2("test-option2", cl::cat(TestCategory),
-   cl::cat(cl::GeneralCategory),
-   cl::cat(cl::GeneralCategory));
+   cl::cat(cl::getGeneralCategory()),
+   cl::cat(cl::getGeneralCategory()));
 
-  // Make sure cl::GeneralCategory wasn't added twice.
+  // Make sure cl::getGeneralCategory() wasn't added twice.
   ASSERT_EQ(TestOption2.Categories.size(), 2U);
 
   ASSERT_NE(TestOption2.Categories.end(),
@@ -166,9 +166,9 @@
   << "Failed to assign Option Category.";
   ASSERT_NE(TestOption2.Categories.end(),
 find_if(TestOption2.Categories,
- [&](const llvm::cl::OptionCategory *Cat) {
-   return Cat == &cl::GeneralCategory;
- }))
+[&](const llvm::cl::OptionCategory *Cat) {
+  return Cat == &cl::getGeneralCategory();
+}))
   << "Failed to assign General Category.";
 
   cl::OptionCategory AnotherCategory("Additional test Options", "Description");
@@ -176,9 +176,9 @@
   cl::cat(AnotherCategory));
   ASSERT_EQ(TestOption.Categories.end(),
 find_if(TestOption.Categories,
- [&](const llvm::cl::OptionCategory *Cat) {
-   return Cat == &cl::GeneralCategory;
- }))
+[&](const llvm::cl::OptionCategory *Cat) {
+  return Cat == &cl::getGeneralCategory();
+}))
   << "Failed to remove General Category.";
   ASSERT_NE(TestOption.Categories.end(),
 find_if(TestOption.Categories,
Index: llvm/unittests/Support/ARMAttributeParser.cpp
===
--- llvm/unittests/Support/ARMAttributeParser.cpp
+++ llvm/unittests/Support/ARMAttributeParser.cpp
@@ -48,7 +48,7 @@
 }
 
 bool testTagString(unsigned Tag, const char *name) {
-  return ELFAttrs::attrTypeAsS

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 transparently only for libSupport, as it contains the 
entry point for command line parsing it can explicitly trigger the registration 
of the options for itself. Other libraries in LLVM don't have this luxury...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105959/new/

https://reviews.llvm.org/D105959

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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:
  https://reviews.llvm.org/D105959?vs=359196&id=359201#toc

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
  clang-tools-extra/clangd/indexer/IndexerMain.cpp
  clang/tools/clang-refactor/ClangRefactor.cpp
  llvm/docs/CommandLine.rst
  llvm/include/llvm/Support/ARMAttributeParser.h
  llvm/include/llvm/Support/ARMBuildAttributes.h
  llvm/include/llvm/Support/CommandLine.h
  llvm/include/llvm/Support/RISCVAttributeParser.h
  llvm/include/llvm/Support/RISCVAttributes.h
  llvm/include/llvm/Support/ScopedPrinter.h
  llvm/include/llvm/Support/WithColor.h
  llvm/lib/Support/ARMBuildAttrs.cpp
  llvm/lib/Support/CommandLine.cpp
  llvm/lib/Support/Debug.cpp
  llvm/lib/Support/DebugCounter.cpp
  llvm/lib/Support/DebugOptions.h
  llvm/lib/Support/ELFAttributeParser.cpp
  llvm/lib/Support/GraphWriter.cpp
  llvm/lib/Support/RISCVAttributes.cpp
  llvm/lib/Support/RandomNumberGenerator.cpp
  llvm/lib/Support/Signals.cpp
  llvm/lib/Support/Statistic.cpp
  llvm/lib/Support/TimeProfiler.cpp
  llvm/lib/Support/Timer.cpp
  llvm/lib/Support/TypeSize.cpp
  llvm/lib/Support/Windows/Signals.inc
  llvm/lib/Support/WithColor.cpp
  llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
  llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp
  llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
  llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
  llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
  llvm/unittests/Support/ARMAttributeParser.cpp
  llvm/unittests/Support/CommandLineTest.cpp
  llvm/unittests/Support/RISCVAttributeParserTest.cpp

Index: llvm/unittests/Support/RISCVAttributeParserTest.cpp
===
--- llvm/unittests/Support/RISCVAttributeParserTest.cpp
+++ llvm/unittests/Support/RISCVAttributeParserTest.cpp
@@ -49,7 +49,7 @@
 }
 
 static bool testTagString(unsigned Tag, const char *name) {
-  return ELFAttrs::attrTypeAsString(Tag, RISCVAttrs::RISCVAttributeTags)
+  return ELFAttrs::attrTypeAsString(Tag, RISCVAttrs::getRISCVAttributeTags())
  .str() == name;
 }
 
Index: llvm/unittests/Support/CommandLineTest.cpp
===
--- llvm/unittests/Support/CommandLineTest.cpp
+++ llvm/unittests/Support/CommandLineTest.cpp
@@ -110,7 +110,7 @@
   ASSERT_NE(Retrieved->Categories.end(),
 find_if(Retrieved->Categories,
 [&](const llvm::cl::OptionCategory *Cat) {
-  return Cat == &cl::GeneralCategory;
+  return Cat == &cl::getGeneralCategory();
 }))
   << "Incorrect default option category.";
 
@@ -152,10 +152,10 @@
 
 TEST(CommandLineTest, UseMultipleCategories) {
   StackOption TestOption2("test-option2", cl::cat(TestCategory),
-   cl::cat(cl::GeneralCategory),
-   cl::cat(cl::GeneralCategory));
+   cl::cat(cl::getGeneralCategory()),
+   cl::cat(cl::getGeneralCategory()));
 
-  // Make sure cl::GeneralCategory wasn't added twice.
+  // Make sure cl::getGeneralCategory() wasn't added twice.
   ASSERT_EQ(TestOption2.Categories.size(), 2U);
 
   ASSERT_NE(TestOption2.Categories.end(),
@@ -166,9 +166,9 @@
   << "Failed to assign Option Category.";
   ASSERT_NE(TestOption2.Categories.end(),
 find_if(TestOption2.Categories,
- [&](const llvm::cl::OptionCategory *Cat) {
-   return Cat == &cl::GeneralCategory;
- }))
+[&](const llvm::cl::OptionCategory *Cat) {
+  return Cat == &cl::getGeneralCategory();
+}))
   << "Failed to assign General Category.";
 
   cl::OptionCategory AnotherCategory("Additional test Options", "Description");
@@ -176,9 +176,9 @@
   cl::cat(AnotherCategory));
   ASSERT_EQ(TestOption.Categories.end(),
 find_if(TestOption.Categories,
- [&](const llvm::cl::OptionCategory *Cat) {
-   return Cat == &cl::GeneralCategory;
- }))
+[&](const llvm::cl::OptionCategory *Cat) {
+  return Cat == &cl::getGeneralCategory();
+}))
   << "Failed to remove General Category.";
   ASSERT_NE(TestOption.Categories.end(),
 find_if(TestOption.Categories,
Index: llvm/unittests/Support/ARMAttributeParser.cpp
==

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

[Mehdi AMINI via Phabricator via cfe-commits]

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

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105959/new/

https://reviews.llvm.org/D105959

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105959/new/

https://reviews.llvm.org/D105959

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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/D121076/new/

https://reviews.llvm.org/D121076

Files:
  mlir/python/mlir/dialects/_scf_ops_ext.py
  mlir/test/python/dialects/scf.py

Index: mlir/test/python/dialects/scf.py
===
--- mlir/test/python/dialects/scf.py
+++ mlir/test/python/dialects/scf.py
@@ -82,3 +82,58 @@
 # CHECK:   iter_args(%{{.*}} = %[[ARGS]]#0, %{{.*}} = %[[ARGS]]#1)
 # CHECK: scf.yield %{{.*}}, %{{.*}}
 # CHECK:   return
+
+
+@constructAndPrintInModule
+def testIfWithoutElse():
+  bool = IntegerType.get_signless(1)
+  i32 = IntegerType.get_signless(32)
+
+  @builtin.FuncOp.from_py_func(bool)
+  def simple_if(cond):
+if_op = scf.IfOp(cond)
+with InsertionPoint(if_op.then_block):
+  one = arith.ConstantOp(i32, 1)
+  add = arith.AddIOp(one, one)
+  scf.YieldOp([])
+return
+
+
+# CHECK: func @simple_if(%[[ARG0:.*]]: i1)
+# CHECK: scf.if %[[ARG0:.*]]
+# CHECK:   %[[ONE:.*]] = arith.constant 1
+# CHECK:   %[[ADD:.*]] = arith.addi %[[ONE]], %[[ONE]]
+# CHECK: return
+
+
+@constructAndPrintInModule
+def testIfWithElse():
+  bool = IntegerType.get_signless(1)
+  i32 = IntegerType.get_signless(32)
+
+  @builtin.FuncOp.from_py_func(bool)
+  def simple_if_else(cond):
+if_op = scf.IfOp(cond, [i32, i32], hasElse=True)
+with InsertionPoint(if_op.then_block):
+  x_true = arith.ConstantOp(i32, 0)
+  y_true = arith.ConstantOp(i32, 1)
+  scf.YieldOp([x_true, y_true])
+with InsertionPoint(if_op.else_block):
+  x_false = arith.ConstantOp(i32, 2)
+  y_false = arith.ConstantOp(i32, 3)
+  scf.YieldOp([x_false, y_false])
+add = arith.AddIOp(if_op.results[0], if_op.results[1])
+return
+
+
+# CHECK: func @simple_if_else(%[[ARG0:.*]]: i1)
+# CHECK: %[[RET:.*]]:2 = scf.if %[[ARG0:.*]]
+# CHECK:   %[[ZERO:.*]] = arith.constant 0
+# CHECK:   %[[ONE:.*]] = arith.constant 1
+# CHECK:   scf.yield %[[ZERO]], %[[ONE]]
+# CHECK: } else {
+# CHECK:   %[[TWO:.*]] = arith.constant 2
+# CHECK:   %[[THREE:.*]] = arith.constant 3
+# CHECK:   scf.yield %[[TWO]], %[[THREE]]
+# CHECK: arith.addi %[[RET]]#0, %[[RET]]#1
+# CHECK: return
Index: mlir/python/mlir/dialects/_scf_ops_ext.py
===
--- mlir/python/mlir/dialects/_scf_ops_ext.py
+++ mlir/python/mlir/dialects/_scf_ops_ext.py
@@ -64,3 +64,44 @@
 To obtain the loop-carried operands, use `iter_args`.
 """
 return self.body.arguments[1:]
+
+
+class IfOp:
+  """Specialization for the SCF if op class."""
+
+  def __init__(self,
+   cond,
+   results_=[],
+   *,
+   hasElse=False,
+   loc=None,
+   ip=None):
+"""Creates an SCF `if` operation.
+
+- `cond` is a MLIR value of 'i1' type to determine which regions of code will be executed.
+- `hasElse` determines whether the if operation has the else branch.
+"""
+operands = []
+operands.append(cond)
+results = []
+results.extend(results_)
+super().__init__(
+self.build_generic(
+regions=2,
+results=results,
+operands=operands,
+loc=loc,
+ip=ip))
+self.regions[0].blocks.append(*[])
+if hasElse:
+self.regions[1].blocks.append(*[])
+
+  @property
+  def then_block(self):
+"""Returns the then block of the if operation."""
+return self.regions[0].blocks[0]
+
+  @property
+  def else_block(self):
+"""Returns the else block of the if operation."""
+return self.regions[1].blocks[0]
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121549: Define ABI breaking class members correctly

[Mehdi AMINI via Phabricator via cfe-commits]

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 
`include` directory)

Same for the SystemZ one above I believe.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121549/new/

https://reviews.llvm.org/D121549

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121549: Define ABI breaking class members correctly

[Mehdi AMINI via Phabricator via cfe-commits]

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-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 ACTION
  https://reviews.llvm.org/D123297/new/

https://reviews.llvm.org/D123297

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119136/new/

https://reviews.llvm.org/D119136

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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
> commit a fix in a few hours

OK, I'll revert in the meantime to unbreak bots.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119136/new/

https://reviews.llvm.org/D119136

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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` etc).

Why can't you build the object files with `-Os` in the first place instead of 
changing the optimization level between the compilation and the linking phases?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72404/new/

https://reviews.llvm.org/D72404

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 above until you collaborate actively to find a supported way and move there 
though. 
Just asking "please move away from the library usage" alone won't be enough.


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@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 only look at 
> function attributes instead of `SizeLevel`? Because at the moment, it's a 
> weird mix of both. IMHO size level should either all go via function 
> attributes or via a flag, not something in between as it is now.

I agree, I don't know (other than history) why we couldn't move towards 
removing `PassManagerBuilder.SizeLevel`?

> Also, if size level is done via function attributes, why not optimization 
> level? There is already `optnone`. I'm not saying that's better, but right 
> now I don't see the logic in this whole system.

Despite what gcc and clang exposes to their users, at the LLVM level we don't 
have a single dimension on which to put `O1/O2/O3` compared to `Os/Oz`. These 
also may not make sense for every single compiler out-there: `O1/O2/O3` for 
clang may not be the right pass pipeline for my proprietary shader compiler.
Another reason why O1 /O2 
/O3 
 are not making much such to be in 
the IR is that the IR is intended to be stored and reloaded any time in the 
middle of pipeline. LTO is an example of this, so we don't really want to store 
the "list of pass to run" in the IR.
Finally, we don't want to (or we can't really...) teach passes about whether 
they should execute during O1 /O2 
/O3 
, while `optnone` is just a "fuse" 
to disable them all. It is also convenient to have `optnone` as a function 
attribute because it allows to selectively disable the optimizer on a per 
function basis. On the other hand because of the nature of the pass pipeline, 
it can't be tweaked on a per function basis (what about Module passes?).

On the other hand the optsize/minsize are driving heuristic and can be 
orthogonal to the pass pipeline / controlled on a per-function basis and made 
available to every pass: they convey an "optimization goal" that applies to 
every pass individually.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72404/new/

https://reviews.llvm.org/D72404

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 for 
the operands.
From the revision description: `, (1) the operands defined by non-constant-like 
ops come first, followed by (2) block arguments, and these are followed by (3) 
the operands defined by constant-like ops` which all seems like a fairly local 
check: a stable-sort on the operands would be deterministic and local to a 
single operation.




Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:235
+  bfsOfOperand->key = (Twine(bfsOfOperand->key) + Twine("1") +
+   
std::string(frontAncestor->getName().getStringRef()))
+  .str();

The string conversion seems unnecessary to me?



Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:271
+  for (Operation *operandDefOp : operandDefOps) {
+OperandBFS *bfsOfOperand = new OperandBFS();
+bfsOfOperand->pushAncestor(operandDefOp);

Is this a leak?



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124750/new/

https://reviews.llvm.org/D124750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mehdi AMINI via Phabricator via cfe-commits]

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 hand side" is there already.
>
> I think this was not added to canonicalization because we wanted it to be an 
> independent utility that can be used if needed and not be used if not needed.

You're telling me "what" while I'm actually more interested in the "why" here?

>> 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 
>> for the operands.
>> From the revision description: (1) the operands defined by non-constant-like 
>> ops come first, followed by (2) block arguments, and these are followed by 
>> (3) the operands defined by constant-like ops` which all seems like a fairly 
>> local check: a stable-sort on the operands would be deterministic and local 
>> to a single operation.
>
> I do this because, firstly, in the description, if you look below this 
> paragraph, you will see the following:
> "And, if two operands come from the same op, the function backtracks and
> looks even further to sort them. This backtracking is done over the
> backward slice of the operand, in a breadth-first traversal."

Same as before: this does not tell me why, can you provide an example where 
this matters?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124750/new/

https://reviews.llvm.org/D124750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits