[Lldb-commits] [PATCH] D137838: [Support] Move TargetParsers to new component

[Francesco Petrogalli via Phabricator via lldb-commits]

fpetrogalli added a comment.

In D137838#4010987 , @dblaikie wrote:

> This has introduced a circular dependency due to the forwarding header 
> (forwarding header depends on new lib, new lib depends on support, where the 
> forwarding header is). Generally this wouldn't be acceptable (& I'd suggest 
> the patch be reverted on that basis) though I understand this is a 
> complicated migration - what's the timeline for correcting this issue?

IIUC, the fix here would be to remove the forwarding headers, right?

Francesco


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137838

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

[Lldb-commits] [PATCH] D137838: [Support] Move TargetParsers to new component

[Francesco Petrogalli via Phabricator via lldb-commits]

fpetrogalli accepted this revision.
fpetrogalli added a comment.

In D137838#4000959 , @lenary wrote:

> [...] I think this is ready to land on Monday?

SGTM - thank you again!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137838

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

[Lldb-commits] [PATCH] D137838: [RFC][Support] Move TargetParsers to new component

[Francesco Petrogalli via Phabricator via lldb-commits]

fpetrogalli accepted this revision.
fpetrogalli added a comment.
This revision is now accepted and ready to land.

Thank you for working on this @lenary  - LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137838

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

[Lldb-commits] [PATCH] D137838: [RFC][Support] Move TargetParsers to new component

[Francesco Petrogalli via Phabricator via lldb-commits]

fpetrogalli added a comment.

@lenary - thank you for the update!

I have added a bunch of miso comments, a bit repetitive... by the time I 
realised they were repetitive it was faster to get to the bottom of it than 
removing them!

In D137838#3931295 , @lenary wrote:

> I've updated the patch to use forwarding headers (and to rebase past some 
> changes that have happened in the interim).
>
> The patch is still huge because of the number of places using the 
> TargetParsers, which need the component added to their CMakeLists.txt.
>
> I think after the next release, I will add `#warning` to the forwarding 
> headers, so their usage can be noticed and updated.

Why not add the `#warning` directive now?

Thank you again,

Francesco




Comment at: clang/lib/Lex/CMakeLists.txt:3
 
-set(LLVM_LINK_COMPONENTS support)
+set(LLVM_LINK_COMPONENTS
+  Support

I wonder how `support` was not being detected as a linking error...



Comment at: clang/lib/Tooling/DumpTool/CMakeLists.txt:5
   ClangSrcLocDump.cpp
-)
+  )
 

nit: unrelated change.



Comment at: flang/tools/bbc/CMakeLists.txt:2-4
+  Passes
+  TargetParser
+  )

nit: extra tabs



Comment at: flang/unittests/Optimizer/CMakeLists.txt:14
   ${dialect_libs}
 )
 

Why not add `LLVMTargetParser` here?



Comment at: llvm/include/llvm/ADT/Triple.h:1
-//===-- llvm/ADT/Triple.h - Target triple helper class --*- C++ 
-*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===--===//
-
-#ifndef LLVM_ADT_TRIPLE_H
-#define LLVM_ADT_TRIPLE_H
-
-#include "llvm/ADT/Twine.h"
-#include "llvm/Support/VersionTuple.h"
-
-// Some system headers or GCC predefined macros conflict with identifiers in
-// this file.  Undefine them here.
-#undef NetBSD
-#undef mips
-#undef sparc
-
-namespace llvm {
-
-/// Triple - Helper class for working with autoconf configuration names. For
-/// historical reasons, we also call these 'triples' (they used to contain
-/// exactly three fields).
-///
-/// Configuration names are strings in the canonical form:
-///   ARCHITECTURE-VENDOR-OPERATING_SYSTEM
-/// or
-///   ARCHITECTURE-VENDOR-OPERATING_SYSTEM-ENVIRONMENT
-///
-/// This class is used for clients which want to support arbitrary
-/// configuration names, but also want to implement certain special
-/// behavior for particular configurations. This class isolates the mapping
-/// from the components of the configuration name to well known IDs.
-///
-/// At its core the Triple class is designed to be a wrapper for a triple
-/// string; the constructor does not change or normalize the triple string.
-/// Clients that need to handle the non-canonical triples that users often
-/// specify should use the normalize method.
-///
-/// See autoconf/config.guess for a glimpse into what configuration names
-/// look like in practice.
-class Triple {
-public:
-  enum ArchType {
-UnknownArch,
-
-arm,// ARM (little endian): arm, armv.*, xscale
-armeb,  // ARM (big endian): armeb
-aarch64,// AArch64 (little endian): aarch64
-aarch64_be, // AArch64 (big endian): aarch64_be
-aarch64_32, // AArch64 (little endian) ILP32: aarch64_32
-arc,// ARC: Synopsys ARC
-avr,// AVR: Atmel AVR microcontroller
-bpfel,  // eBPF or extended BPF or 64-bit BPF (little endian)
-bpfeb,  // eBPF or extended BPF or 64-bit BPF (big endian)
-csky,   // CSKY: csky
-dxil,   // DXIL 32-bit DirectX bytecode
-hexagon,// Hexagon: hexagon
-loongarch32,// LoongArch (32-bit): loongarch32
-loongarch64,// LoongArch (64-bit): loongarch64
-m68k,   // M68k: Motorola 680x0 family
-mips,   // MIPS: mips, mipsallegrex, mipsr6
-mipsel, // MIPSEL: mipsel, mipsallegrexe, mipsr6el
-mips64, // MIPS64: mips64, mips64r6, mipsn32, mipsn32r6
-mips64el,   // MIPS64EL: mips64el, mips64r6el, mipsn32el, mipsn32r6el
-msp430, // MSP430: msp430
-ppc,// PPC: powerpc
-ppcle,  // PPCLE: powerpc (little endian)
-ppc64,  // PPC64: powerpc64, ppu
-ppc64le,// PPC64LE: powerpc64le
-r600,   // R600: AMD GPUs HD2XXX - HD6XXX
-amdgcn, // AMDGCN: AMD GCN GPUs
-riscv32,// RISC-V (32-bit): riscv32
-riscv64,// RISC-V (64-bit): riscv64
-sparc,  // Sparc: sparc
-sparcv9,// Sparcv9: Sparcv9
-sparcel,// Sparc: (endianness = little). NB: 'Sparcle' is a CPU 
variant
-systemz,// SystemZ: 

[Lldb-commits] [PATCH] D137838: [RFC][Support] Move TargetParsers to new component

[Francesco Petrogalli via Phabricator via lldb-commits]

fpetrogalli added a comment.

Hi @lenary  - thank you for working on this!

The patch is reasonable to me.

I agree on the suggestion of using forwarding headers or the first iteration of 
the change, it will make it easier to review.

I'll adapt the auto-get patch at D137517  on 
D137838 .

As for the name `TargetParser`. @arsenm came up with the name 
`SubtargetRegistry`. I am fine with either names.

Francesco


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137838

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

[Lldb-commits] [PATCH] D82187: [AArch64][SVE] ACLE: Add bfloat16 to struct load/stores.

[Francesco Petrogalli via Phabricator via lldb-commits]

fpetrogalli added inline comments.



Comment at: clang/include/clang/Basic/AArch64SVEACLETypes.def:69
 
-SVE_VECTOR_TYPE("__SVBFloat16_t", "__SVBFloat16_t", SveBFloat16, 
SveBFloat16Ty, 8, 16, false, false, true)
+SVE_VECTOR_TYPE("__SVBFloat16_t", "__SVBFloat16_t", SveBFloat16, 
SveBFloat16Ty, 8, 16, true, false, true)
 

Why did you have to set `IsFP = true`? Seems like an unrelated change?



Comment at: clang/utils/TableGen/SveEmitter.cpp:541
 Float = false;
+BFloat = false;
 ElementBitwidth /= 4;

Are these needed? I don't understand the rule for when to be specific on the 
values of these variables.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82187



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

[Lldb-commits] [PATCH] D71761: [lldb] Add a setting to not install the main executable

[Francesco Petrogalli via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7c9bcba644c4: [lldb] Add a setting to not install the main 
executable (authored by fpetrogalli).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71761

Files:
  lldb/include/lldb/Target/Target.h
  
lldb/packages/Python/lldbsuite/test/commands/target/auto-install-main-executable/Makefile
  
lldb/packages/Python/lldbsuite/test/commands/target/auto-install-main-executable/TestAutoInstallMainExecutable.py
  
lldb/packages/Python/lldbsuite/test/commands/target/auto-install-main-executable/main.cpp
  lldb/source/Target/Target.cpp
  lldb/source/Target/TargetProperties.td

Index: lldb/source/Target/TargetProperties.td
===
--- lldb/source/Target/TargetProperties.td
+++ lldb/source/Target/TargetProperties.td
@@ -154,6 +154,9 @@
   def RequireHardwareBreakpoints: Property<"require-hardware-breakpoint", "Boolean">,
 DefaultFalse,
 Desc<"Require all breakpoints to be hardware breakpoints.">;
+  def AutoInstallMainExecutable: Property<"auto-install-main-executable", "Boolean">,
+DefaultTrue,
+Desc<"Always install the main executable when connected to a remote platform.">;
 }
 
 let Definition = "process" in {
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -2691,8 +2691,10 @@
   if (platform_sp) {
 if (platform_sp->IsRemote()) {
   if (platform_sp->IsConnected()) {
-// Install all files that have an install path, and always install the
-// main executable when connected to a remote platform
+// Install all files that have an install path when connected to a
+// remote platform. If target.auto-install-main-executable is set then
+// also install the main executable even if it does not have an explicit
+// install path specified.
 const ModuleList  = GetImages();
 const size_t num_images = modules.GetSize();
 for (size_t idx = 0; idx < num_images; ++idx) {
@@ -2703,10 +2705,8 @@
 if (local_file) {
   FileSpec remote_file(module_sp->GetRemoteInstallFileSpec());
   if (!remote_file) {
-if (is_main_executable) // TODO: add setting for always
-// installing main executable???
-{
-  // Always install the main executable
+if (is_main_executable && GetAutoInstallMainExecutable()) {
+  // Automatically install the main executable.
   remote_file = platform_sp->GetRemoteWorkingDirectory();
   remote_file.AppendPathComponent(
   module_sp->GetFileSpec().GetFilename().GetCString());
@@ -3970,6 +3970,12 @@
   m_collection_sp->SetPropertyAtIndexAsBoolean(nullptr, idx, b);
 }
 
+bool TargetProperties::GetAutoInstallMainExecutable() const {
+  const uint32_t idx = ePropertyAutoInstallMainExecutable;
+  return m_collection_sp->GetPropertyAtIndexAsBoolean(
+  nullptr, idx, g_target_properties[idx].default_uint_value != 0);
+}
+
 void TargetProperties::Arg0ValueChangedCallback() {
   m_launch_info.SetArg0(GetArg0());
 }
Index: lldb/packages/Python/lldbsuite/test/commands/target/auto-install-main-executable/main.cpp
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/commands/target/auto-install-main-executable/main.cpp
@@ -0,0 +1,8 @@
+#include 
+
+const char* build = BUILD;
+
+int main(int argc, char **argv) {
+  printf("argc: %d\n", argc);
+  return argv[0][0];
+}
Index: lldb/packages/Python/lldbsuite/test/commands/target/auto-install-main-executable/TestAutoInstallMainExecutable.py
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/commands/target/auto-install-main-executable/TestAutoInstallMainExecutable.py
@@ -0,0 +1,137 @@
+"""
+Test target commands: target.auto-install-main-executable.
+"""
+
+import time
+import gdbremote_testcase
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestAutoInstallMainExecutable(gdbremote_testcase.GdbRemoteTestCaseBase):
+mydir = TestBase.compute_mydir(__file__)
+
+def setUp(self):
+super(TestAutoInstallMainExecutable, self).setUp()
+self._initial_platform = lldb.DBG.GetSelectedPlatform()
+
+def tearDown(self):
+lldb.DBG.SetSelectedPlatform(self._initial_platform)
+super(TestAutoInstallMainExecutable, self).tearDown()
+
+@llgs_test
+@no_debug_info_test
+@skipIf(remote=False)
+@expectedFailureAll(hostoslist=["windows"], triple='.*-android')
+def