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

mehdi_amini added a comment. In D124750#3502401 , @srishti-pm wrote: > In D124750#3502295 , @mehdi_amini > wrote: > >> You're telling me "what" while I'm actually more interested in the "why" >> here? > > I'm n

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

mehdi_amini added a comment. That was the sense of my question about canonicalization indeed :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124750/new/ https://reviews.llvm.org/D124750 ___ cfe-commits

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

mehdi_amini added a comment. Thanks for improving the doc! Are you moving this to be used in canonicalization next? I think a first good step would be to make it a pattern and test it with a pass that applies it in the greedy rewriter. I would land this first and then try to enable this in the

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

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

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

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

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

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

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

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

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

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

[PATCH] D85596: [Docs] Fix --print-supported-cpus option rendering

This revision was automatically updated to reflect the committed changes. Closed by commit rG0fb2203cd6c2: [Docs] Fix --print-supported-cpus option rendering (authored by tmfink, committed by mehdi_amini). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org

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

mehdi_amini added a comment. Overall that would likely work for XLA. Something I'd like to mention though in response to: > The veclib type is also tied to the accepted values for -fveclib, which is a > list of supported lib, `-fveclib` is a Clang thing, it shouldn't limit what LLVM does. Of c

[PATCH] D91488: Consider reference, pointer, and pointer-to-membber TemplateArguments to be different if they have different types.

mehdi_amini added a comment. Clang is crashing after this change when bootstrapping and building lib/Support/CMakeFiles/LLVMSupport.dir/Hashing.cpp.o (our log if it helps: https://buildkite.com/mlir/mlir-core/builds/10003#80a5b6a9-d8d9-4420-ab2a-c24f398bc5d4 ) Repository: rG LLVM Github Mon

[PATCH] D91980: [OpenMP] Add initial support for `omp [begin/end] assumes`

mehdi_amini added inline comments. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:101 +#include "llvm/Frontend/OpenMP/OMPKinds.def" +}; + This is broken on gcc-5: ``` llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:101:1: error: could not convert

[PATCH] D82860: Port ObjCMTAction to new option parsing system

mehdi_amini added a comment. Reverted in f917356f9ce0 ; I suspect a `static constexpr` in a class missing a definition out of class (required pre-c++17). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION ht

[PATCH] D84293: Add an assertion in SmallVector::push_back()

mehdi_amini updated this revision to Diff 305279. mehdi_amini added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. Fix one clang instance failing this assertion Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84293/n

[PATCH] D84293: Add an assertion in SmallVector::push_back()

mehdi_amini added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:10811-10813 +Ops.reserve(Ops.size() + 1); Ops.push_back(Ops[0]); Ops.erase(Ops.begin()); njames93 wrote: > Probably a little off topic, but shouldn't this be refactored a

[PATCH] D84293: Add an assertion in SmallVector::push_back()

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG2c196bbc6bd8: Add an assertion in SmallVector::push_back() (authored by mehdi_amini). Changed prior to commit: https://reviews.llvm.org/D84293?vs=

[PATCH] D87981: [X86] AMX programming model.

mehdi_amini added a comment. (Drive by comments) Comment at: llvm/include/llvm/CodeGen/TileShapeInfo.h:27 + +class ShapeT { +public: Can you document the class? Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:30 + +class X86PreTileConfig

[PATCH] D83088: Introduce CfgTraits abstraction

mehdi_amini added a comment. In D83088#2345540 , @nhaehnle wrote: > David, I don't think this is appropriate here. Let's take the discussion to > llvm-dev. Seems like David asked to revert in the meantime? Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D83088: Introduce CfgTraits abstraction

mehdi_amini added a comment. In D83088#2347111 , @arsenm wrote: > In D83088#2346322 , @mehdi_amini > wrote: > >> In D83088#2345540 , @nhaehnle wrote: >> >>> David, I don't t

[PATCH] D83088: Introduce CfgTraits abstraction

mehdi_amini added a comment. In D83088#2350287 , @nhaehnle wrote: > Hi Mehdi, this is not an appropriate place for this discussion. Yes, we have > a general rule that patches can be reverted if they're obviously broken (e.g. > build bot problems) or clea

[PATCH] D95766: [Branch-Rename] Fix some links

mehdi_amini accepted this revision. mehdi_amini added inline comments. Comment at: clang-tools-extra/clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.h:20 // Check for underscores in the names of googletest tests, per -// https://github.com/google/googletest/blob/master/

[PATCH] D109977: LLVM Driver Multicall tool

mehdi_amini added a comment. That's pretty nice! Have you thought about looking into a lit option (triggered by a cmake flag maybe) that would change the substitution for the tools that are enabled by llvm-driver to use the latter instead when running the tests? Repository: rG LLVM Github Mo

[PATCH] D107349: [Matrix] Overload stride arg in matrix.columnwise.load/store.

mehdi_amini added a comment. Revert to unbreak bots (like this one : https://lab.llvm.org/buildbot/#/builders/13/builds/10930 ) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107349/new/ https://reviews.llvm.org/D107349 ___

[PATCH] D108265: .clang-tidy: Push variable related readability-identifier-naming options down to projects

mehdi_amini added a comment. +1 on arguments from @dblaikie : I'm not sure I understand the motivation for this change right now? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108265/new/ https://reviews.llvm.org/D108265 _

[PATCH] D108265: .clang-tidy: Push variable related readability-identifier-naming options down to projects

mehdi_amini added a comment. > the original discussion that spawned the (not-yet-accepted, as it says in its > opening sentence) linked proposal around variable naming. +1 with David again: if the proposal gets accepted, then this patch makes sense to me. Repository: rG LLVM Github Monorepo

[PATCH] D114082: [WIP] Normalize String Attributes

mehdi_amini added a comment. The patch is pretty large, any chance you could split in two? First the change introducing the AttributeKey and changing the API, and then updating all the callees separately as a follow up? Comment at: llvm/include/llvm/IR/Attributes.h:84 + } +}

[PATCH] D114394: Compile-time computation of string attribute hashes

mehdi_amini added inline comments. Comment at: llvm/lib/IR/Attributes.cpp:125 FoldingSetNodeID ID; - ID.AddString(Kind); + ID.AddString(Kind.value()); if (!Val.empty()) ID.AddString(Val); Carrying over my comment from the original revision: it seem that y

[PATCH] D114502: File Reorganization changes

mehdi_amini added inline comments. Comment at: mlir/lib/ExecutionEngine/CMakeLists.txt:153 + set(CMAKE_MODULE_PATH "${HIP_PATH}/lib/cmake/hip" ${CMAKE_MODULE_PATH}) find_package(HIP) if (NOT HIP_FOUND) Is MLIR really coupled to Clang here? That seems suspi

[PATCH] D114502: File Reorganization changes

mehdi_amini added inline comments. Comment at: mlir/lib/ExecutionEngine/CMakeLists.txt:153 + set(CMAKE_MODULE_PATH "${HIP_PATH}/lib/cmake/hip" ${CMAKE_MODULE_PATH}) find_package(HIP) if (NOT HIP_FOUND) mehdi_amini wrote: > Is MLIR really coupled to Clang h

[PATCH] D114639: Raise the minimum Visual Studio version to VS2019

mehdi_amini added a comment. In D114639#3158401 , @RKSimon wrote: >> Have you checked whether there are any bots in the lab that will need to be >> updated? > > I did find a number of bots that I think need addressing, I am intending to > privately emai

[PATCH] D114699: Fixed a memory leak in the PDLToPDLInterp RootOrderingTest.

mehdi_amini added inline comments. Comment at: mlir/unittests/Conversion/PDLToPDLInterp/RootOrderingTest.cpp:37 + ~RootOrderingTest() { +for (int i = 0; i < 4; ++i) + v[i].getDefiningOp()->destroy(); bondhugula wrote: > bondhugula wrote: > > You need to

[PATCH] D114639: Raise the minimum Visual Studio version to VS2019

mehdi_amini added a comment. In D114639#3161303 , @erichkeane wrote: > IMO, if we're updating the MSVC versions, we should do the same for the > GCC/Clang/AppleClang versions too. For example, GCC 5.1 is from 2015, and > Clang 3.5 is from 2014. We've

[PATCH] D114639: Raise the minimum Visual Studio version to VS2019

mehdi_amini added a comment. In D114639#3162069 , @erichkeane wrote: > In D114639#3162032 , @RKSimon wrote: > >> In D114639#3162000 , @mehdi_amini >> wrote: >> >>> In D1

[PATCH] D114639: Raise the minimum Visual Studio version to VS2019

mehdi_amini added a comment. In D114639#3162141 , @erichkeane wrote: >> Right, but last time we did the motivation was specifically to get to c++14, >> while here the motivation is to drop an old MSVC according to the >> MSVC-specific support we intend

[PATCH] D134878: Update developer policy on potentially breaking changes

mehdi_amini added a comment. From what I saw when you posted the discourse thread initially, I understand you're targeting user-visible features, and mostly from the "toolchain" side of the project. However I find the language here to be potentially confusing for the API surface of the librarie

[PATCH] D131172: [clang][llvm][doc] Add more information for the ABI change in FP16

mehdi_amini added inline comments. Comment at: clang/docs/ReleaseNotes.rst:633 +- If you are using downstream runtimes that provide FP16 conversions, update +them with the new ABI. + Can you add a link to a documentation that describe the new ABI? Repository:

[PATCH] D131172: [clang][llvm][doc] Add more information for the ABI change in FP16

mehdi_amini added inline comments. Comment at: llvm/docs/ReleaseNotes.rst:206 +When you find failures with ``half`` type, check the calling conversion of the +code and switch it to the new ABI. + (same here: users reading "switch it to the new ABI" will be confus

[PATCH] D140896: [WIP] Move from llvm::makeArrayRef to ArrayRef deduction guides

mehdi_amini added inline comments. Herald added a subscriber: Michael137. Comment at: llvm/include/llvm/ADT/ArrayRef.h:502 + /// Deduction guide to construct an ArrayRef from a C array. + template ArrayRef(const T (&Arr)[N]) -> ArrayRef; Can we keep the make

[PATCH] D141298: Move from llvm::makeArrayRef to ArrayRef deduction guides - last part

mehdi_amini added a comment. Seems mechanical, and if it build everywhere LGTM :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141298/new/ https://reviews.llvm.org/D141298 ___ cfe-commits mailing list c

[PATCH] D107082: [X86][RFC] Enable `_Float16` type support on X86 following the psABI

mehdi_amini added a comment. This broke the bot here: https://lab.llvm.org/buildbot/#/builders/61/builds/27616 The cmake invocation includes some GPU specific options that you can omit (`-DMLIR_ENABLE_CUDA_RUNNER=1` , `-DCMAKE_CUDA_COMPILER=/usr/local/cuda/bin/nvcc`, `-DMLIR_ENABLE_VULKAN_RUN

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

mehdi_amini added inline comments. Comment at: mlir/include/mlir/Transforms/CommutativityUtils.h:211 +template +class SortCommutativeOperands : public OpRewritePattern { + using OpRewritePattern::OpRewritePattern; Can we make this not a template? This will be a

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

mehdi_amini added a comment. Right now I'm not yet understanding all of the algorithm (haven't spent enough time on it), but I'm mostly concerned by the runtime cost of this normalization. Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:371 +// assigned a sort

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

mehdi_amini added a comment. I hit a config failure after f8c8bda965dd0f622de1ad3863b728661af6eb72 CMake Error at /var/lib/buildkite-agent/builds/buildkite-588bd64db9-mk2vq-1/mlir/mlir-core/libcxx/CMakeLists.txt:225 (messa

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

mehdi_amini added a comment. Forgot to mention, we configure this build with: cmake -G Ninja ${src_dir}/llvm -DLLVM_TARGETS_TO_BUILD="X86;NVPTX;AMDGPU" -DLLVM_ENABLE_PROJECTS="lld;clang;mlir" -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=Off -DLIBCXX_CXX_ABI=default -DLLVM_ENABLE_RUNTI

[PATCH] D126158: [MLIR][GPU] Replace fdiv on fp16 with promoted (fp32) multiplication with reciprocal plus one (conditional) Newton iteration.

mehdi_amini added a comment. The shared library build was broken, I had to revert: https://lab.llvm.org/buildbot/#/builders/61/builds/27377 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126158/new/ https://reviews.llvm.org/D126158 ___

[PATCH] D155509: Revert "Remove rdar links; NFC"

mehdi_amini created this revision. mehdi_amini added reviewers: lattner, aaron.ballman. Herald added subscribers: steakhal, jdoerfert, martong, pengfei, arphaman. Herald added a reviewer: NoQ. Herald added a project: All. mehdi_amini requested review of this revision. Herald added subscribers: cfe-

[PATCH] D155509: Revert "Remove rdar links; NFC"

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGe0ac46e69d7a: Revert "Remove rdar links; NFC" (authored by mehdi_amini). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://

[PATCH] D95299: Fix truncated __OPENMP_NVPTX__ preprocessor condition

mehdi_amini added a comment. Herald added a subscriber: wangpc. @r-burns : what should I use for the authorship (name/pseudo and email)? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95299/new/ https://reviews.llvm.org/D95299 _

[PATCH] D95299: Fix truncated __OPENMP_NVPTX__ preprocessor condition

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGc49142e4f5c8: Fix truncated __OPENMP_NVPTX__ preprocessor condition (authored by r-burns, committed by mehdi_amini). Repository: rG LLVM Github Mo

[PATCH] D155081: Specify the developer policy around links to external resources

mehdi_amini accepted this revision. mehdi_amini added a comment. This revision is now accepted and ready to land. Approving in support, but don't consider this enough as-is. I suspect you should get @lattner approval here :) Comment at: llvm/docs/DeveloperPolicy.rst:357 -* I

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

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

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

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

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

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

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

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

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

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

[PATCH] D69498: IR: Invert convergent attribute handling

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

[PATCH] D69498: IR: Invert convergent attribute handling

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

[PATCH] D69498: IR: Invert convergent attribute handling

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

[PATCH] D69498: IR: Invert convergent attribute handling

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

[PATCH] D69498: IR: Invert convergent attribute handling

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

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

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

[PATCH] D69498: IR: Invert convergent attribute handling

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

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

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

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

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

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

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

[PATCH] D141098: [clang-format][NFC] Set LineEnding to LF in config files

mehdi_amini added a comment. In D141098#4111809 , @owenpan wrote: > Can we just fix the buildbot so that it runs git-clang-format with > `--binary`? Using clang-format-15 to format a patch to clang-format-16 source > still looks wrong to me. The genera

[PATCH] D146041: initial commit

mehdi_amini added a comment. Herald added a subscriber: JDevlieghere. You should look into the title and description of the commit: https://cbea.ms/git-commit/ Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146041/new/ https://reviews.llvm.org/D146

[PATCH] D151923: [APFloat] Add APFloat semantic support for TF32

This revision was automatically updated to reflect the committed changes. Closed by commit rG55c2211a233e: [APFloat] Add APFloat semantic support for TF32 (authored by jfurtek, committed by mehdi_amini). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[PATCH] D151923: [APFloat] Add APFloat semantic support for TF32

mehdi_amini added a comment. I think this LG, but David had to update clang/lib/AST/MicrosoftMangle.cpp when some other float format was added, you don't need this here? Also do you have plans for MLIR? (nothing required for this patch, just curious). Repository: rG LLVM Github Monorepo CH

[PATCH] D151923: [APFloat] Add APFloat semantic support for TF32

mehdi_amini added a comment. > Also do you have plans for MLIR? (nothing required for this patch, just > curious). Actually it's the last sentence of the patch description (/facepalm) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151923/new/ http

[PATCH] D151923: [APFloat] Add APFloat semantic support for TF32

mehdi_amini accepted this revision. mehdi_amini added a comment. This revision is now accepted and ready to land. Ok, sorry I missed the mangle update, seeing how I missed the MLIR mention as well: I need more sleep!! :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:/

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

mehdi_amini added inline comments. Herald added a subscriber: bviyer. Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1533 +ompLoc, builder.saveIP(), mapTypeFlags, mapNames, mapperAllocas, +mapperFunc, deviceID, ifCond, processMapO

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

mehdi_amini added inline comments. Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1491 +mapTypes = exitDataOp.getMapTypes(); +mapperFunc = false; +return success(); This line is not needed afte

[PATCH] D27796: Fix printf specifier handling: invalid specifier should not be marked as "consuming data arguments"

mehdi_amini created this revision. mehdi_amini added reviewers: rsmith, bruno, dexonsmith. mehdi_amini added a subscriber: cfe-commits. https://reviews.llvm.org/D27796 Files: clang/include/clang/Analysis/Analyses/FormatString.h clang/test/CodeGen/builtins.c clang/test/Sema/format-strings.c

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

mehdi_amini accepted this revision. mehdi_amini added a comment. This revision is now accepted and ready to land. LGTM. Comment at: lib/CodeGen/CodeGenFunction.cpp:1086 + } + return true; +} Just a nit: reversing the if condition allows early exit. Reposito

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

mehdi_amini added a comment. (Wait a little in case @Quuxplusone still has comments.) Repository: rL LLVM https://reviews.llvm.org/D27163 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-c

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

mehdi_amini added inline comments. Comment at: include/clang/Driver/Options.td:1324 + HelpText<"Use C++ undefined behaviour optimization for control flow paths" + "that reach the end of the function without executing a required return">; +def fno_strict_return : Flag<["-"],

[PATCH] D27796: Fix printf specifier handling: invalid specifier should not be marked as "consuming data arguments"

This revision was automatically updated to reflect the committed changes. Closed by commit rL289850: Fix printf specifier handling: invalid specifier should not be marked as… (authored by mehdi_amini). Changed prior to commit: https://reviews.llvm.org/D27796?vs=81532&id=81621#toc Repository:

[PATCH] D27832: Add -plugin-opt=sample-profile for thinLTO build.

mehdi_amini added a comment. Is this covered by a driver test usually? https://reviews.llvm.org/D27832 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26964: Handle more declarations in DeclContextPrinter to fix -print-decl-contexts crashes

mehdi_amini accepted this revision. mehdi_amini added a comment. This revision is now accepted and ready to land. Testing is in line with the existing testing. It is not great, but at the same time is a development/debugging tool right? Repository: rL LLVM https://reviews.llvm.org/D26964 _

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

mehdi_amini added inline comments. Comment at: llvm/include/llvm/ADT/APFloat.h:800 - void makeLargest(bool Neg) { getIEEE().makeLargest(Neg); } + void makeLargest(bool Neg) { +if (usesLayout(getSemantics())) { jtony wrote: > I know it is allowed to return

[PATCH] D27832: Add -plugin-opt=sample-profile for thinLTO build.

mehdi_amini accepted this revision. mehdi_amini added a comment. This revision is now accepted and ready to land. LGTM, but fix what @bruno spotted before committing. https://reviews.llvm.org/D27832 ___ cfe-commits mailing list cfe-commits@lists.llv

[PATCH] D28047: Remove the '-disable-llvm-passes' flag (which I didn't even know existed, and I suspect many others aren't aware of either) and strength '-disable-llvm-optzns' to do the same thing.

mehdi_amini added a comment. In https://reviews.llvm.org/D28047#629746, @rnk wrote: > How about standardizing on -disable-llvm-passes instead of > -disable-llvm-optzns? I never liked "optzns" and can't remember how to spell > it. Also, this flag disables LLVM instrumentation passes (ASan) as we

[PATCH] D28053: Cleanup the handling of noinline function attributes, -fno-inline, -fno-inline-functions, -O0, and optnone.

mehdi_amini added a comment. > It would be awesome if attribute sets were a bit more FileCheck friendly, but > oh well. I've been wondering about that, what's the point of attribute sets in the textual IR? I understand the idea for saving space in the Bitcode, but the IR does not have to use t

[PATCH] D28053: Cleanup the handling of noinline function attributes, -fno-inline, -fno-inline-functions, -O0, and optnone.

mehdi_amini added a comment. In https://reviews.llvm.org/D28053#629768, @rnk wrote: > The big change here is that `clang -O0` now applies the noinline attribute > everywhere. I can see why someone might expect things to work that way, but > it seems surprising to me at first glance. > > Before

[PATCH] D28053: Cleanup the handling of noinline function attributes, -fno-inline, -fno-inline-functions, -O0, and optnone.

mehdi_amini added inline comments. Comment at: lib/Frontend/CompilerInvocation.cpp:453 +// There is no effect at O0 when we won't inline anyways. +if (Opts.OptimizationLevel > 1) { + const Option &InlineOpt = InlineArg->getOption(); I'd switch the tw

[PATCH] D28053: Cleanup the handling of noinline function attributes, -fno-inline, -fno-inline-functions, -O0, and optnone.

mehdi_amini added inline comments. Comment at: lib/Frontend/CompilerInvocation.cpp:453 +// There is no effect at O0 when we won't inline anyways. +if (Opts.OptimizationLevel > 1) { + const Option &InlineOpt = InlineArg->getOption(); mehdi_amini wrote

[PATCH] D28047: Remove the '-disable-llvm-passes' flag (which I didn't even know existed, and I suspect many others aren't aware of either) and strength '-disable-llvm-optzns' to do the same thing.

mehdi_amini added a comment. LGTM, but please wait for @rnk to confirm :) Also any opinion about a driver option -emit-raw-llvm as @joerg suggested? https://reviews.llvm.org/D28047 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lis

[PATCH] D28053: Cleanup the handling of noinline function attributes, -fno-inline, -fno-inline-functions, -O0, and optnone.

mehdi_amini added inline comments. Comment at: lib/Frontend/CompilerInvocation.cpp:453 +// There is no effect at O0 when we won't inline anyways. +if (Opts.OptimizationLevel > 1) { + const Option &InlineOpt = InlineArg->getOption(); chandlerc wrote:

[PATCH] D28053: Cleanup the handling of noinline function attributes, -fno-inline, -fno-inline-functions, -O0, and optnone.

mehdi_amini added a comment. > That said, I agree with Reid that it is a bit surprising. After thinking > about it a lot, the reason I personally find it surprising is actually that I > find -O0 being the default surprising. But I'm OK with not changing that > here, and maybe never changing tha

[PATCH] D28047: Remove the '-disable-llvm-passes' flag (which I didn't even know existed, and I suspect many others aren't aware of either) and strength '-disable-llvm-optzns' to do the same thing.

mehdi_amini accepted this revision. mehdi_amini added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D28047#629922, @chandlerc wrote: > In https://reviews.llvm.org/D28047#629892, @mehdi_amini wrote: > > > LGTM, but please wait for @rnk to confirm :) > >

[PATCH] D28077: [PM] Introduce options to enable the (still experimental) new pass manager, and a code path to use it.

mehdi_amini accepted this revision. mehdi_amini added a comment. This revision is now accepted and ready to land. LGTM! Exciting to see the new PM arriving in clang :) Comment at: lib/CodeGen/BackendUtil.cpp:757 + setCommandLineOpts(); + cl::PrintOptionValues(); + ---

[PATCH] D28133: add cxa_demangle_fuzzer

mehdi_amini added inline comments. Comment at: fuzz/CMakeLists.txt:3 +if( LLVM_USE_SANITIZE_COVERAGE ) + set(LLVM_LINK_COMPONENTS support) + This is a dependency on libLLVMSupport right? Why is this needed? https://reviews.llvm.org/D28133 __

[PATCH] D28133: add cxa_demangle_fuzzer

mehdi_amini accepted this revision. mehdi_amini added a comment. This revision is now accepted and ready to land. LGTM. (I can't test because LLVM_USE_SANITIZE_COVERAGE seems broken on Darwin) https://reviews.llvm.org/D28133 ___ cfe-commits mailing

[PATCH] D28139: [ThinLTO] No need to rediscover imports in distributed backend

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

[PATCH] D28153: [clang] Fix clean build of generate-order-file

mehdi_amini added inline comments. Comment at: utils/perf-training/CMakeLists.txt:61 +message(FATAL_ERROR "Output clang order file is not set") + endif() + I'm missing something: the code in the main CMakeLists seems to allows to have an empty value for the

[PATCH] D28153: [clang] Fix clean build of generate-order-file

mehdi_amini added inline comments. Comment at: CMakeLists.txt:436 + + if(CLANG_ORDER_FILE AND NOT EXISTS ${CLANG_ORDER_FILE}) +string(FIND "${CLANG_ORDER_FILE}" "${CMAKE_CURRENT_BINARY_DIR}" PATH_START) So why `if(CLANG_ORDER_FILE ` here? Repository: rL

[PATCH] D28153: [clang] Fix clean build of generate-order-file

mehdi_amini added a comment. I mean the two checks being out-of-sync is weird, so I rather have them reconciled. Repository: rL LLVM https://reviews.llvm.org/D28153 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cg

[PATCH] D28153: [clang] Fix clean build of generate-order-file

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

[PATCH] D28362: [ThinLTO] Optionally ignore empty index file

mehdi_amini added inline comments. Comment at: lib/CodeGen/BackendUtil.cpp:65 +"Ignore an empty index file and perform non-ThinLTO compilation"), +llvm::cl::init(false)); + Is it common to do this in clang? https://reviews.llvm.org/D28362 ___

<    1   2   3   4   >