[Lldb-commits] [PATCH] D34550: Fix typo: using && instead of & when evaluating a mask

[Mehdi AMINI via Phabricator via lldb-commits]

mehdi_amini created this revision.
Herald added a subscriber: emaste.

Reported by coverity, I don't know how to provide a test.


https://reviews.llvm.org/D34550

Files:
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp


Index: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1693,7 +1693,7 @@
 // ABI Mask doesn't cover N32 and N64 ABI.
 if (header.e_ident[EI_CLASS] == llvm::ELF::ELFCLASS64)
   arch_flags |= lldb_private::ArchSpec::eMIPSABI_N64;
-else if (header.e_flags && llvm::ELF::EF_MIPS_ABI2)
+else if (header.e_flags & llvm::ELF::EF_MIPS_ABI2)
   arch_flags |= lldb_private::ArchSpec::eMIPSABI_N32;
 break;
   }


Index: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1693,7 +1693,7 @@
 // ABI Mask doesn't cover N32 and N64 ABI.
 if (header.e_ident[EI_CLASS] == llvm::ELF::ELFCLASS64)
   arch_flags |= lldb_private::ArchSpec::eMIPSABI_N64;
-else if (header.e_flags && llvm::ELF::EF_MIPS_ABI2)
+else if (header.e_flags & llvm::ELF::EF_MIPS_ABI2)
   arch_flags |= lldb_private::ArchSpec::eMIPSABI_N32;
 break;
   }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D34550: Fix typo: using && instead of & when evaluating a mask

[Mehdi AMINI via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL306134: Fix typo: using && instead of & when evaluating a 
mask (authored by mehdi_amini).

Changed prior to commit:
  https://reviews.llvm.org/D34550?vs=103690&id=103752#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D34550

Files:
  lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp


Index: lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1693,7 +1693,7 @@
 // ABI Mask doesn't cover N32 and N64 ABI.
 if (header.e_ident[EI_CLASS] == llvm::ELF::ELFCLASS64)
   arch_flags |= lldb_private::ArchSpec::eMIPSABI_N64;
-else if (header.e_flags && llvm::ELF::EF_MIPS_ABI2)
+else if (header.e_flags & llvm::ELF::EF_MIPS_ABI2)
   arch_flags |= lldb_private::ArchSpec::eMIPSABI_N32;
 break;
   }


Index: lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1693,7 +1693,7 @@
 // ABI Mask doesn't cover N32 and N64 ABI.
 if (header.e_ident[EI_CLASS] == llvm::ELF::ELFCLASS64)
   arch_flags |= lldb_private::ArchSpec::eMIPSABI_N64;
-else if (header.e_flags && llvm::ELF::EF_MIPS_ABI2)
+else if (header.e_flags & llvm::ELF::EF_MIPS_ABI2)
   arch_flags |= lldb_private::ArchSpec::eMIPSABI_N32;
 break;
   }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D57330: Adjust documentation for git migration.

[Mehdi AMINI via Phabricator via lldb-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



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

[Lldb-commits] [PATCH] D57330: Adjust documentation for git migration.

[Mehdi AMINI via Phabricator via lldb-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



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

[Lldb-commits] [PATCH] D57330: Adjust documentation for git migration.

[Mehdi AMINI via Phabricator via lldb-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



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

[Lldb-commits] [PATCH] D29288: Switch std::call_once to llvm::call_once

[Mehdi AMINI via Phabricator via lldb-commits]

mehdi_amini added a comment.

I'm fine with this change, but I'll leave the approval to one of the LLDB 
developer :)

(Thanks for following-up with the libstdc++ on these platforms!)


Repository:
  rL LLVM

https://reviews.llvm.org/D29288



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

[Lldb-commits] [PATCH] D29288: Switch std::call_once to llvm::call_once

[Mehdi AMINI via Phabricator via lldb-commits]

mehdi_amini added a comment.

In https://reviews.llvm.org/D29288#660677, @krytarowski wrote:

> In https://reviews.llvm.org/D29288#660636, @clayborg wrote:
>
> > Be very careful when using this, you can't change member variables that 
> > used to be std::once to be statics. We also don't need the llvm namespace 
> > to be included with "using namespace llvm;" in many of the files.
>
>
> `using namespace llvm;` is currently required in order to get this functional:


What about just updating the macro in LLVM to make it not needed?

  #define LLVM_DEFINE_ONCE_FLAG(flag) static llvm::once_flag flag = 
Uninitialized


Repository:
  rL LLVM

https://reviews.llvm.org/D29288



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

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

[Mehdi AMINI via Phabricator via lldb-commits]

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 be erasing those ops using `erase()`. 
> Nit: `unsigned` here and below.
@bondhugula unsigned is an anti-pattern (unless working with bit-fields or 
other bit-manipulation), this has been widely documented I believe, both in C++ 
standardization paper (they are stuck with size_t in the standard library) and 
with conference talks like this one: https://www.youtube.com/watch?v=yG1OZ69H_-o


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114699

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

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

[Mehdi AMINI via Phabricator via lldb-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

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

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

[Mehdi AMINI via Phabricator via lldb-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

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

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

[Mehdi AMINI via Phabricator via lldb-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

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

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

[Mehdi AMINI via Phabricator via lldb-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

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

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

[Mehdi AMINI via Phabricator via lldb-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

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

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

[Mehdi AMINI via Phabricator via lldb-commits]

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 makeArrayRef functions for now and mark them deprecated?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140896

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

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

[Mehdi AMINI via Phabricator via lldb-commits]

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

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

[Lldb-commits] [PATCH] D146041: initial commit

[Mehdi AMINI via Phabricator via lldb-commits]

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

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

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

[Mehdi AMINI via Phabricator via lldb-commits]

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 libraries: that is how much of this applies to the LLVM C++ 
libraries API surface?
If this is out-of-scope, can this be called out more explicitly?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134878

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

[Lldb-commits] [PATCH] D98179: [lit] Sort test start times based on prior test timing data

[Mehdi AMINI via Phabricator via lldb-commits]

mehdi_amini added inline comments.
Herald added a subscriber: JDevlieghere.



Comment at: llvm/utils/lit/lit/cl_arguments.py:157
 selection_group.add_argument("-i", "--incremental",
-help="Run modified and failing tests first (updates mtimes)",
+help="Start modified and failing tests first (updates mtimes)",
 action="store_true")

davezarzycki wrote:
> mehdi_amini wrote:
> > Seems like you deprecated the option, can you update the doc to reflect it?
> I still can't find any documentation for `--incremental`. What am I missing?
Sorry I wasn't clear: I meant the "help" field here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98179

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

[Lldb-commits] [PATCH] D98179: [lit] Sort test start times based on prior test timing data

[Mehdi AMINI via Phabricator via lldb-commits]

mehdi_amini added a comment.

> Can we revert to the previous behavior please? The current behavior is not 
> user friendly. Thanks!

To be clear: you car about the order in the final summary, not the actually 
execution order, right?

How are you diffing? Copy-pasting the terminal output to a file? (I alway pipe 
these kind of list through `sort` before diffing them)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98179

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

[Lldb-commits] [PATCH] D98179: [lit] Sort test start times based on prior test timing data

[Mehdi AMINI via Phabricator via lldb-commits]

mehdi_amini added a comment.

> No, I'm running lit and dumping into a file. sorting is not as easy as piping 
> through sort, as I'm running with -vv (required).

Since tests run in parallel, don't you already have some non-determinism in the 
-vv output? Isn't it printing as test finish? (just curious, not pushing back 
on anything here).

> Anyway, sorting the list on lit side should be 1 line for some that knows 
> where that line goes :)

Yeah I agree that sorting on the lit side for the summary should be fairly easy!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98179

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

[Lldb-commits] [PATCH] D79219: [CMake] Simplify CMake handling for zlib

[Mehdi AMINI via Phabricator via lldb-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



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

[Lldb-commits] [PATCH] D79219: [CMake] Simplify CMake handling for zlib

[Mehdi AMINI via Phabricator via lldb-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



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

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

[Mehdi AMINI via Phabricator via lldb-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



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

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

[Mehdi AMINI via Phabricator via lldb-commits]

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/googletest/docs/faq.md#why-should-test-suite-names-and-test-names-not-contain-underscore
 ///

You removed the wrong word here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95766

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

[Lldb-commits] [PATCH] D80130: [mlir][SystemZ] Fix incompatible datalayout in SystemZ

[Mehdi AMINI via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9f2ce5b915a5: [mlir][SystemZ] Fix incompatible datalayout in 
SystemZ (authored by imaihal, committed by mehdi_amini).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80130

Files:
  mlir/lib/ExecutionEngine/ExecutionEngine.cpp


Index: mlir/lib/ExecutionEngine/ExecutionEngine.cpp
===
--- mlir/lib/ExecutionEngine/ExecutionEngine.cpp
+++ mlir/lib/ExecutionEngine/ExecutionEngine.cpp
@@ -27,6 +27,7 @@
 #include "llvm/ExecutionEngine/Orc/RTDyldObjectLinkingLayer.h"
 #include "llvm/ExecutionEngine/SectionMemoryManager.h"
 #include "llvm/IR/IRBuilder.h"
+#include "llvm/MC/SubtargetFeature.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/Host.h"
@@ -119,8 +120,17 @@
 errs() << "NO target: " << errorMessage << "\n";
 return true;
   }
-  std::unique_ptr machine(
-  target->createTargetMachine(targetTriple, "generic", "", {}, {}));
+
+  std::string cpu(llvm::sys::getHostCPUName());
+  llvm::SubtargetFeatures features;
+  llvm::StringMap hostFeatures;
+
+  if (llvm::sys::getHostCPUFeatures(hostFeatures))
+for (auto &f : hostFeatures)
+  features.AddFeature(f.first(), f.second);
+
+  std::unique_ptr machine(target->createTargetMachine(
+  targetTriple, cpu, features.getString(), {}, {}));
   llvmModule->setDataLayout(machine->createDataLayout());
   llvmModule->setTargetTriple(targetTriple);
   return false;


Index: mlir/lib/ExecutionEngine/ExecutionEngine.cpp
===
--- mlir/lib/ExecutionEngine/ExecutionEngine.cpp
+++ mlir/lib/ExecutionEngine/ExecutionEngine.cpp
@@ -27,6 +27,7 @@
 #include "llvm/ExecutionEngine/Orc/RTDyldObjectLinkingLayer.h"
 #include "llvm/ExecutionEngine/SectionMemoryManager.h"
 #include "llvm/IR/IRBuilder.h"
+#include "llvm/MC/SubtargetFeature.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/Host.h"
@@ -119,8 +120,17 @@
 errs() << "NO target: " << errorMessage << "\n";
 return true;
   }
-  std::unique_ptr machine(
-  target->createTargetMachine(targetTriple, "generic", "", {}, {}));
+
+  std::string cpu(llvm::sys::getHostCPUName());
+  llvm::SubtargetFeatures features;
+  llvm::StringMap hostFeatures;
+
+  if (llvm::sys::getHostCPUFeatures(hostFeatures))
+for (auto &f : hostFeatures)
+  features.AddFeature(f.first(), f.second);
+
+  std::unique_ptr machine(target->createTargetMachine(
+  targetTriple, cpu, features.getString(), {}, {}));
   llvmModule->setDataLayout(machine->createDataLayout());
   llvmModule->setTargetTriple(targetTriple);
   return false;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D80130: [mlir][SystemZ] Fix incompatible datalayout in SystemZ

[Mehdi AMINI via Phabricator via lldb-commits]

mehdi_amini accepted this revision.
mehdi_amini added inline comments.



Comment at: mlir/lib/ExecutionEngine/ExecutionEngine.cpp:123
   }
-  std::unique_ptr machine(
-  target->createTargetMachine(targetTriple, "generic", "", {}, {}));
+  std::string cpu = std::string(llvm::sys::getHostCPUName());
+  llvm::SubtargetFeatures features;

You should be able to write just `std::string cpu = 
llvm::sys::getHostCPUName();`? (Or `std::string 
cpu(llvm::sys::getHostCPUName());`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80130



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