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

2022-12-07 Thread Fangrui Song via Phabricator via lldb-commits 
MaskRay accepted this revision.
MaskRay added a comment.

Please check these builds all work:

- default
- `-DLLVM_LINK_LLVM_DYLIB=on`
- `-DBUILD_SHARED_LIBS=on`


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

2022-12-07 Thread 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

2022-12-07 Thread Fangrui Song via Phabricator via lldb-commits 
MaskRay added a comment.

The name `llvm/lib/TargetParser` 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

2022-12-07 Thread Sam Elliott via Phabricator via lldb-commits 
lenary added a comment.

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

@fpetrogalli I'm going to stick with TargetParser, because I think this is less 
confusing, given that TargetRegistry is already a component.


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

2022-12-07 Thread Sam Elliott via Phabricator via lldb-commits 
lenary added inline comments.



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

fpetrogalli wrote:
> I wonder how `support` was not being detected as a linking error...
Not sure, but I still think getting the case right is useful in this patch.



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

fpetrogalli wrote:
> nit: extra tabs
I've undone the re-indent here.



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

fpetrogalli wrote:
> Why not add `LLVMTargetParser` here?
That's better, done.



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: s390x
-tce,// TCE (http://tce.cs.tut.fi/): tce
-tcele,  // TCE little endian (http://tce.cs.tut.fi/): tcele
-thumb,  // Thumb (little endian): thumb, thumbv.*
-thumbeb,// Thumb (big endian): thumbeb
-x86,// X86: i[3-9]86
-x86_64, // X86-64: amd64, x86_64
-xcore,  // XCore: xcore
-nvptx,  // NVPTX: 32-bit
-nvptx64,// NVPTX: 64-bit
-le32,   // le32: generic little-endian 32-bit CPU (PNaCl)
-le64,   // le64: generic little-endian 64-bit CPU (PNaCl)
-amdil,  // AMDIL
-amdil64,// AMDIL with 64-bit pointers
-hsail,  // AMD HSAIL
-hsa

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

2022-12-07 Thread 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: s39

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

2022-12-07 Thread Sam Elliott via Phabricator via lldb-commits 
lenary added a comment.

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.


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

2022-12-07 Thread Fangrui Song via Phabricator via lldb-commits 
MaskRay added a comment.

Moving target-specific parsers outside LLVMSupport LGTM. I even objected a bit 
when more stuff of this kind was introduced into LLVMSupport.

+1 for adding temporary forwarding headers for now to avoid update all 
`#include` users.


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

2022-12-07 Thread Sam Elliott via Phabricator via lldb-commits 
lenary added a comment.

In D137838#3921828 , @thakis wrote:

> This is a gigantic diff. I'd recommend keeping the .h files in the old place 
> for v0 and make them just forwarding headers that include the header at the 
> new location. That way, you don't have to update oodles of include lines in 
> this patch, and it makes it a bit easier to see what's going on. (You can 
> then update all the include lines in a trivial follow-up if this change goes 
> through, and then remove the forwarding headers in Support, to cut the 
> dependency you want to remove.)

Thanks for the suggestion. I do agree this patch is too big to land as-is. I 
think this patch is useful as "this shows the full effect of this change", even 
if we find ways to make how we land this patch less invasive. One other thought 
I had was to move the unittests first, but that doesn't make as big a 
difference as the fact that `llvm/ADT/Triple.h` is referenced everywhere.

I would like more comments (either here or on Discourse, link in a prior 
comment) on whether this split is reasonable before I post a v2 of this patch, 
though.


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

2022-12-07 Thread 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] D137838: [RFC][Support] Move TargetParsers to new component

2022-12-07 Thread Nico Weber via Phabricator via lldb-commits 
thakis added a comment.

This is a gigantic diff. I'd recommend keeping the .h files in the old place 
for v0 and make them just forwarding headers that include the header at the new 
location. That way, you don't have to update oodles of include lines in this 
patch, and it makes it a bit easier to see what's going on. (You can then 
update all the include lines in a trivial follow-up if this change goes 
through, and then remove the forwarding headers in Support, to cut the 
dependency you want to remove.)


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

2022-12-07 Thread Sam Elliott via Phabricator via lldb-commits 
lenary added a comment.
Herald added a subscriber: Michael137.

Apologies to everyone who has automatically been marked as a reviewer for this 
change. There is more context for it here: 
https://discourse.llvm.org/t/targetparser-auto-generation-of-riscv-cpu-definitions/66419/4?u=lenary


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