[PATCH] D131141: [RISCV] Add MC support of RISCV Zcb Extension

jrtc27 added inline comments. Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:169 + +let isCompressOnly = true in { + This feels wrong to me, but decompression is a bit dodgy if you can have all of Zcb without some of the extensions that its instructions d

[PATCH] D132486: SONAME introduce option CLANG_FORCE_MATCHING_LIBCLANG_SOVERSION

jrtc27 added a comment. In D132486#3743461 , @h-vetinari wrote: > Thanks for the review. Given that you have concerns, could you voice them in > a larger forum > (https://discourse.llvm.org/t/rationale-for-removing-versioned-libclang-middle-ground-to-k

[PATCH] D132486: SONAME introduce option CLANG_FORCE_MATCHING_LIBCLANG_SOVERSION

jrtc27 requested changes to this revision. jrtc27 added inline comments. This revision now requires changes to proceed. Comment at: clang/CMakeLists.txt:467 +option(CLANG_FORCE_MATCHING_LIBCLANG_SOVERSION + "Force the SOVERSION of libclang to be equal to CLANG_MAJOR" OFF) +

[PATCH] D132247: [clang] Fix emitVoidPtrVAArg for non-zero default alloca address space

jrtc27 created this revision. jrtc27 added a reviewer: arsenm. Herald added subscribers: kosarev, tpr. Herald added a project: All. jrtc27 requested review of this revision. Herald added subscribers: cfe-commits, wdng. Herald added a project: clang. Indirect arguments are passed on the stack and s

[PATCH] D131677: [clang][RISCV] Fix incorrect ABI lowering for inherited structs under hard-float ABIs

jrtc27 added inline comments. Comment at: clang/test/CodeGen/RISCV/riscv-abi.cpp:1 // RUN: %clang_cc1 -triple riscv32 -emit-llvm -x c++ %s -o - \ // RUN: | FileCheck -check-prefixes=ILP32,ILP32-ILP32F,ILP32-ILP32F-ILP32D %s Not sure why you need -x c++ when i

[PATCH] D131677: [clang][RISCV] Fix incorrect ABI lowering for inherited structs under hard-float ABIs

jrtc27 added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:10986 +bool Ret = detectFPCCEligibleStructHelper( +B.getType(), CurOff, Field1Ty, Field1Off, Field2Ty, Field2Off); +if (!Ret) craig.topper wrote: > jrtc27 wrote

[PATCH] D131677: [clang][RISCV] Fix incorrect ABI lowering for inherited structs under hard-float ABIs

jrtc27 added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:10986 +bool Ret = detectFPCCEligibleStructHelper( +B.getType(), CurOff, Field1Ty, Field1Off, Field2Ty, Field2Off); +if (!Ret) jrtc27 wrote: > With multiple inhe

[PATCH] D131677: [clang][RISCV] Fix incorrect ABI lowering for inherited structs under hard-float ABIs

jrtc27 added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:10986 +bool Ret = detectFPCCEligibleStructHelper( +B.getType(), CurOff, Field1Ty, Field1Off, Field2Ty, Field2Off); +if (!Ret) With multiple inheritance this off

[PATCH] D43630: [Driver] Fix search paths on x32

jrtc27 abandoned this revision. jrtc27 added a comment. Herald added a subscriber: pengfei. Herald added a project: All. Superseded by D52050 Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D43630/new/ https://reviews.llvm.org/D4363

[PATCH] D129824: [RISCV] Set triple based on -march flag which can be deduced in more generic way

jrtc27 added inline comments. Comment at: clang/test/Driver/riscv-arch.c:410 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV64-TARGET %s +// RUN: %clang --target=riscv32-unknown-elf -mcpu=sifive-s21 -### %s \ +// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV64-TAR

[PATCH] D129824: [RISCV] Set triple based on -march flag which can be deduced in more generic way

jrtc27 added a comment. If you still want to pursue this, discussion belongs at https://github.com/riscv-non-isa/riscv-toolchain-conventions, not here, since it's an interface shared by Clang and GCC and the two should be consistent CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129824/

[PATCH] D125272: [clang] Add -fcheck-new support

jrtc27 added inline comments. Comment at: clang/test/CodeGenCXX/fcheck-new.cpp:3 +// RUN: %clang_cc1 -fcheck-new -triple x86_64-linux-gnu -S -disable-O0-optnone \ +// RUN: -emit-llvm -o - %s | opt -S -mem2reg | FileCheck %s + MaskRay wrote: > Please remove `opt`

[PATCH] D129824: [RISCV] Set triple based on -march flag which can be deduced in more generic way

jrtc27 added a comment. Does GCC allow this or not? Because this strikes me as a bad idea at first sight… Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129824/new/ https://reviews.llvm.org/D129824 ___ c

[PATCH] D129802: [DRAFT] Implementing new atomic orderings in LLVM and generate barriers for legacy __sync builtins. Support corresponding memory model in outline atomics as well.

jrtc27 added a comment. I can't obviously see a description of what the additional barriers implied by the sync variants is (which should be in an update to LangRef at the very least, if not also in the summary itself). Inferring it from the AArch64 assembly is also difficult, and the RISC-V lo

[PATCH] D129065: [RISCV][Driver] Add libm linking to `RISCVToolchain`

jrtc27 added a comment. Commit subject/body should say C++ in it Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129065/new/ https://reviews.llvm.org/D129065 ___ cfe-commits mailing list cfe-commits@lists.

[PATCH] D128726: [RISCV][NFC] Move static global variables into static variable in function.

jrtc27 added a comment. Seems you're right, the C++11 standard does clearly say (9.7p4): > Dynamic initialization of a block-scope variable with static storage duration > (6.6.4.1) or thread storage duration (6.6.4.2) is performed the first time > control passes through its declaration; such a

[PATCH] D128726: [RISCV][NFC] Move static global variables into static variable in function.

jrtc27 added a comment. The commit message is poor, because the problem is not the use of static globals, it's the use of globals that have constructors. Moving them to be static function-scoped variables doesn't change the fact that they still have static duration; you change the scope of them

[PATCH] D128612: RISC-V big-endian support implementation

jrtc27 added inline comments. Comment at: clang/lib/Basic/Targets/RISCV.h:113 +if (Triple.isLittleEndian()) + resetDataLayout("e-m:e-p:32:32-i64:64-n32-S128"); +else And please avoid repeating the whole data layout, just make the e/E a variable ===

[PATCH] D125947: [RISCV] Add default ABI for archs with only F extension

jrtc27 added a comment. Also, the tests where you have codegen changes rather than preserving a soft-float ABI should probably be put up for review separately by adding an explicit hard single-float ABI, as those seem worthwhile for reducing noise Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D125947: [RISCV] Add default ABI for archs with only F extension

jrtc27 added a comment. It's currently this way in order to be compatible with GCC. Changing this requires consensus from both toolchains to ensure compatibility is preserved. See https://github.com/riscv-non-isa/riscv-toolchain-conventions/issues/13 for some discussion on this. Repository:

[PATCH] D125272: [clang] Add -fcheck-new support

jrtc27 added inline comments. Comment at: clang/test/CodeGenCXX/fcheck-new.cpp:2 +// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py +// RUN: %clang -fcheck-new --target=x86_64-linux-gnu -emit-llvm -S %s -o - | opt -S -mem2reg | FileCheck %s +

[PATCH] D125272: [clang] Add -fcheck-new support

jrtc27 added inline comments. Comment at: clang/test/CodeGenCXX/fcheck-new.cpp:1 +// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py +// RUN: %clang -fcheck-new --target=x86_64-linux-gnu -emit-llvm -S %s -o - | opt -S -mem2reg | FileCheck %s --

[PATCH] D125272: [clang] Add -fcheck-new support

jrtc27 added inline comments. Comment at: clang/test/CodeGenCXX/fcheck-new.cpp:2 +// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py +// RUN: %clang -fcheck-new -emit-llvm -S %s -o - -O2 | FileCheck %s + heatd wrote: > jrtc27 wrote: > >

[PATCH] D125272: [clang] Add -fcheck-new support

jrtc27 added inline comments. Comment at: clang/test/CodeGenCXX/fcheck-new.cpp:2 +// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py +// RUN: %clang -fcheck-new -emit-llvm -S %s -o - -O2 | FileCheck %s + Do you really want -O2 or do you

[PATCH] D122556: [RISCV] Add definitions for Xiangshan processors.

jrtc27 added inline comments. Comment at: llvm/include/llvm/Support/RISCVTargetParser.def:34 PROC(SIFIVE_U74, {"sifive-u74"}, FK_64BIT, {"rv64gc"}) +PROC(XIANGSHAN_YANQIHU,{"xiangshan-yanqihu"},FK_64BIT,{"rv64gc"}) +PROC(XIANGSHAN_NANHU,{"xiangshan-nanhu"},FK_64BIT,{"rv64imafdc_

[PATCH] D122335: [clang] Emit crash reproduction as a single tar file

jrtc27 added a comment. As a developer who often deals with crashes locally this is more annoying; currently I can just point tools at the shell script and C file in /tmp and let them go to work reducing, but now I have to also extract the files CHANGES SINCE LAST ACTION https://reviews.llvm

[PATCH] D120967: [NFC] Divide tests into smaller files

jrtc27 added a comment. With one exception, every RISC-V Clang CodeGen test (and with 9 exceptions, every RISC-V LLVM CodeGen test) is kebab-case not snake_case Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120967/new/ https://reviews.llvm.org/D12

[PATCH] D93298: [RISCV] add the MC layer support of Zfinx extension

jrtc27 added a comment. Never mind, I see you added a test for that case Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93298/new/ https://reviews.llvm.org/D93298 ___ cfe-commits mailing list cfe-commits@

[PATCH] D93298: [RISCV] add the MC layer support of Zfinx extension

jrtc27 added a comment. Does a double with `r` for RV32 work with that fix? That's supposed to give the low half of the register. You might need to also deal with the register pair class? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93298/new/ h

[PATCH] D93298: [RISCV] add the MC layer support of Zfinx extension

jrtc27 added inline comments. Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:10923-10924 - return TargetLowering::getRegForInlineAsmConstraint(TRI, Constraint, VT); + std::pair Res; + Res = TargetLowering::getRegForInlineAsmConstraint(TRI, Constraint, VT); + ---

[PATCH] D120639: [RISCV] Pass -mno-relax to assembler when -fno-integrated-as specified

jrtc27 added inline comments. Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:764 CmdArgs.push_back(MArchName.data()); +if (Args.hasArg(options::OPT_mno_relax)) + CmdArgs.push_back("-mno-relax"); I doubt this does the right thing for `-mrelax -mno-r

[PATCH] D118987: [analyzer] Add failing test case demonstrating buggy taint propagation

jrtc27 added a comment. In D118987#3319697 , @steakhal wrote: > It seems like the `clang-ve-ninja` doesn't really want to accept any patches > from me :D > I hope it's not personal. Let's be friends bot, please. > > Link to the breakage: https://lab.llvm

[PATCH] D93298: [RISCV] add the MC layer support of Zfinx extension

jrtc27 added a comment. In D93298#3332038 , @xbolva00 wrote: >>> It appears that this is causing an assertion segfault in a rustc test over >>> at our experimental rust + llvm@head bot: > > I dont think that patch author is required to debug this issue fo

[PATCH] D93298: [RISCV] add the MC layer support of Zfinx extension

jrtc27 added a comment. In D93298#3332020 , @achieveartificialintelligence wrote: > @krasimir Since I don't have a rust environment, can you help me to test if > D120130 works? Hm, actually, looking at the log, it does

[PATCH] D93298: [RISCV] add the MC layer support of Zfinx extension

jrtc27 added a comment. In D93298#3331641 , @krasimir wrote: > It appears that this is causing an assertion segfault in a `rustc` test over > at our experimental rust + llvm@head bot: > https://buildkite.com/llvm-project/rust-llvm-integrate-prototype/buil

[PATCH] D119788: [AArch64] Add support for -march=native for Apple M1 CPU

jrtc27 added inline comments. Comment at: llvm/lib/Support/Host.cpp:1320 + int Error = sysctlbyname("hw.cpufamily", &Family, &Length, NULL, 0); + assert(!Error && "Fetching hw.cpufamily failed"); Not sure this should be an assert? I think other implementation

[PATCH] D116774: AST: Move __va_list tag back to std conditionally on AArch64.

jrtc27 added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:8555-8556 +// namespace std { struct __va_list { +NamespaceDecl *NS; +NS = NamespaceDecl::Create(const_cast(*Context), + Context->getTranslationUnitDecl(), ---

[PATCH] D116773: AST: Make getEffectiveDeclContext() a member function of ItaniumMangleContextImpl. NFCI.

jrtc27 accepted this revision. jrtc27 added a comment. This revision is now accepted and ready to land. This makes sense to me but I don't know if you want someone with more authority in these parts to review it Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.

[PATCH] D116774: AST: Move __va_list tag back to std conditionally on AArch64.

jrtc27 accepted this revision. jrtc27 added a comment. This revision is now accepted and ready to land. Thanks, looks good to me Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116774/new/ https://reviews.llvm.org/D116774 ___

[PATCH] D118333: [RISCV] Use computeTargetABI from llc as well as clang

jrtc27 added inline comments. Comment at: llvm/test/CodeGen/RISCV/double-mem.ll:2 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py -; RUN: llc -mtriple=riscv32 -mattr=+d -verify-machineinstrs < %s \ +; RUN: llc -mtriple=riscv32 -mattr=+d -target-abi=

[PATCH] D116774: AST: Move __va_list tag back to std conditionally on AArch64.

jrtc27 added inline comments. Comment at: clang/lib/AST/ItaniumMangle.cpp:641 +getASTContext().getTargetInfo().getTriple().getArch(); +if (Arch == llvm::Triple::aarch64 || Arch == llvm::Triple::aarch64_be || +Arch == llvm::Triple::arm || Arch == llvm::Triple::

[PATCH] D116774: AST: Move __va_list tag to the top level on ARM architectures.

jrtc27 added a comment. Ping? I'd really like to get this fixed in 14. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116774/new/ https://reviews.llvm.org/D116774 ___ cfe-commits mailing list cfe-commits@

[PATCH] D117929: [XRay] Add support for RISCV

jrtc27 added inline comments. Comment at: compiler-rt/lib/xray/xray_riscv.cpp:122-123 + //lui t2, %highest(__xray_FunctionEntry/Exit) + //slli t2, t2, 32 ;lui sign extends values + //srli t2, t2, 32 ;

[PATCH] D118333: [RISCV] Update computeTargetABI implementation.

jrtc27 added a comment. Uh D113959 even Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118333/new/ https://reviews.llvm.org/D118333 ___ cfe-commits mailing list cfe-comm

[PATCH] D118333: [RISCV] Update computeTargetABI implementation.

jrtc27 added a comment. I think this is the same idea as D118333 ? Other than being a cleaner way of achieving the same goal. I've not looked to see if there are any functional differences between the two. Repository: rG LLVM Github Monorepo CHANGES SINCE L

[PATCH] D116774: AST: Move __va_list tag to the top level on ARM architectures.

jrtc27 added a comment. Ping? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116774/new/ https://reviews.llvm.org/D116774 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi

[PATCH] D117929: [XRay] Add support for RISCV

jrtc27 added a comment. In D117929#3263462 , @ashwin98 wrote: > Thank you for your feedback! I could combine the riscv32 and 64 cpp files > with some xlen conditions if that will work better, but that might take a bit > of a hit in terms of readability

[PATCH] D117929: [XRay] Add support for RISCV

jrtc27 added a comment. 1. Please upload patches with full context 2. You should not need to have separate xray_riscv32/64.cpp, a single shared file with a small amount of XLEN-based conditions should suffice and reduce a whole load of code duplication. Possibly also applies to the trampoline a

[PATCH] D98574: [Sparc] Don't define __sparcv9 and __sparcv9__ when targeting V8+

jrtc27 accepted this revision. jrtc27 added a comment. This revision is now accepted and ready to land. It's been a while so I've since forgotten the details, but this looks fine now to me Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98574/new/ h

[PATCH] D116774: AST: Move __va_list tag to the top level on ARM architectures.

jrtc27 added a comment. The fact that va_list is in the std namespace does leak out into __builtin_dump_struct, possibly the odd other place, and of course to AST consumers. I think it'd make sense to keep ASTContext as putting it in the std namespace for C++ (like it does for Arm, and used to

[PATCH] D116425: [clang-tidy] Improve modernize-redundant-void-arg to recognize macro uses

jrtc27 added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp:561 +#define return_t(T) T +return_t(void) func(void); +// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: redundant void argument list in function declaration --

[PATCH] D104830: AST: Create __va_list in the std namespace even in C.

jrtc27 added a comment. Ping? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104830/new/ https://reviews.llvm.org/D104830 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

jrtc27 added inline comments. Comment at: llvm/lib/Support/RISCVISAInfo.cpp:61 {"zvlsseg", RISCVExtensionVersion{0, 10}}, +//{"zvlsseg", RISCVExtensionVersion{0, 7}}, This one too?.. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115921/new/

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

jrtc27 added a comment. In D115921#3224324 , @zixuan-wu wrote: > In D115921#3224284 , @jrtc27 wrote: > >> but also with RISC-V extensions not being changed once ratified any more >> (changes mean new extensions e

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

jrtc27 added a comment. I'm unconvinced about landing something like this until there's an actual use case in the tree. How do we know this will actually work the way we want it to if there's nothing proving it? It's still unclear to me how exactly this is going to be represented in the target

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

jrtc27 added a comment. Do not bring back V draft 0.7. It is gone, it will never be supported again by LLVM under that name. The standard extension namespace is reserved for ratified extensions and development snapshots only, not old drafts vendors decided to ship. For those, non-standard exten

[PATCH] D104830: AST: Create __va_list in the std namespace even in C.

jrtc27 added a comment. Ping? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104830/new/ https://reviews.llvm.org/D104830 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi

[PATCH] D94355: [Passes] Add relative lookup table converter pass

jrtc27 added inline comments. Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:395 +/// in 64-bit achitectures where 32-bit offsets might not be enough. +if (TM.getCodeModel() == CodeModel::Medium || +TM.getCodeModel() == CodeModel::Large) The

[PATCH] D94355: [Passes] Add relative lookup table converter pass

jrtc27 added inline comments. Comment at: llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp:132 + // Place new instruction sequence after GEP. + Builder.SetInsertPoint(GEP); + Value *Index = GEP->getOperand(2); This line causes the bug seen in bind. In tha

[PATCH] D113237: [RISCV] Support I extension version 2.1

jrtc27 added a comment. Outside of the I/F/D special cases, where F/D don't really matter and I2p0 is just I2p1Zicsr2p0_Zifencei2p0,, I thought the new policy was that ratified extensions would never be changed, only new extensions published, and thus version numbers are basically irrelevant ot

[PATCH] D104830: AST: Create __va_list in the std namespace even in C.

jrtc27 added a comment. Any luck with this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104830/new/ https://reviews.llvm.org/D104830 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://list

[PATCH] D64008: [RISCV] Avoid save-restore target feature warning

jrtc27 added a comment. Herald added subscribers: VincentWu, luke957, achieveartificialintelligence, vkmr, frasercrmck, evandro, luismarques, sameer.abuasal, s.egerton, MaskRay. For historical documentation purposes: I don't think this ever actually fully worked. Whilst it stops passing the fea

[PATCH] D114533: LLVM IR should allow bitcast between address spaces with the same size.

jrtc27 added a comment. In D114533#3154225 , @arsenm wrote: > In D114533#3153924 , @jrtc27 wrote: > >> This seems like it should not apply to non-integral address spaces? > > No, it shouldn’t care about the indivi

[PATCH] D114533: LLVM IR should allow bitcast between address spaces with the same size.

jrtc27 added a comment. This seems like it should not apply to non-integral address spaces? Comment at: llvm/docs/LangRef.rst:10999 +with this instruction. Conversion of pointers to different address spaces +are legal, but may result in undefined behaviour where such a reinter

[PATCH] D114408: Fold a lot of ffixed_x if judgments

jrtc27 requested changes to this revision. jrtc27 added a comment. This revision now requires changes to proceed. This has clearly not been tested whatsoever, it’s totally broken, the preprocessor does not work like that Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:/

[PATCH] D104830: AST: Create __va_list in the std namespace even in C.

jrtc27 added a comment. Great, thanks Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104830/new/ https://reviews.llvm.org/D104830 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm

[PATCH] D104830: AST: Create __va_list in the std namespace even in C.

jrtc27 added a comment. Ping? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104830/new/ https://reviews.llvm.org/D104830 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi

[PATCH] D104830: AST: Create __va_list in the std namespace even in C.

jrtc27 added a comment. This also leaks out to users via __builtin_dump_struct (https://godbolt.org/z/vx3rjdPdq), and of course the DWARF having the namespace in it will result in users debugging plain C seeing C++ namespaces. ASTDiagnostic's Desugar happens to special-case va_list so it doesn'

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

jrtc27 added a comment. If it's bypassing the descriptors then __builtin_symbol_address is the wrong name (and a bit ambiguous). As far as dlsym is concerned, the symbol is the descriptor, but when you get down to the ELF representation itself that's not always true. For PPC64 ELFv1, the ELF sy

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

jrtc27 added a comment. In D108479#3110003 , @rjmccall wrote: > `std::addressof(&someFunction)` certainly ought to return a signed pointer > under ptrauth, so if your goal here is to get a completely unadorned symbol > address, I think you do need a dif

[PATCH] D104830: AST: Create __va_list in the std namespace even in C.

jrtc27 added a comment. I also note that 32-bit Arm uses the same namespacing, and has CFI-icall support (at least, it's enabled, I don't know if it's actually expected to work) but did not have this change made to it (though probably best not to do so until the DWARF issue is resolved) Repos

[PATCH] D104830: AST: Create __va_list in the std namespace even in C.

jrtc27 added a comment. Or you can still do it in the frontend, just make sure that the DWARF emitted by the frontend doesn't have the DINamespace wrapper when not compiling C++. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104830/new/ https://re

[PATCH] D104830: AST: Create __va_list in the std namespace even in C.

jrtc27 added a comment. This results in the DWARF type info containing DW_TAG_namespace for C, which breaks DTrace's CTF (as the name implies, it is for C, not C++). This does not seem correct to me. If your mangling for va_list is broken then you should special case that in the CFI mangler as

[PATCH] D112890: headers: optionalise some generated resource headers

jrtc27 added a comment. And I don't see any AArch64 or ARM maintainers ACKing this. You haven't even tagged any as reviewers. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112890/new/ https://reviews.llvm.org/D112890 _

[PATCH] D112890: headers: optionalise some generated resource headers

jrtc27 added a comment. This change seems pretty counter to Clang's ability to spit out IR for anything regardless of what backends you have. The right fix should be to shrink the headers, not mask the problem by changing the principles of how Clang works. Repository: rG LLVM Github Monorepo

[PATCH] D109372: [RISCV][RFC] Add Clang support for RISC-V overlay system

jrtc27 added a comment. In D109372#3099947 , @edward-jones wrote: > I reverted some of the previous changes I made so that this patch matches the > spec as currently written - this means it's two attributes again, and the > diagnostic messages have bee

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

jrtc27 added a comment. In D105168#3071321 , @craig.topper wrote: > In D105168#3071249 , @jrtc27 wrote: > >> Two options come to mind if we really need to be outputting a StringRef list: >> >> 1. (the simpler opt

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

jrtc27 added a comment. Two options come to mind if we really need to be outputting a StringRef: 1. (the simpler option) Pass in a Twine -> `const char *` lambda that the caller hooks up to Args.MakeArgString 2. (probably the nicer option) Invent our own MakeArgString that allocates from storag

[PATCH] D111062: [RISCV] Rename some assembler mnemonic and intrinsic functions for RVV 1.0.

jrtc27 added inline comments. Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:1372 +(VMANDN_MM VR:$vd, VR:$vs2, VR:$vs1), 0>; + +def : InstAlias<"vmornot.mm $vd, $vs2, $vs1", craig.topper wrote: > Probably not worth having a blank line betwee

[PATCH] D111692: [RISCV] Remove Zvamo C intrinsics and builtins.

jrtc27 added a comment. I don't have a problem with deleting the code if there's a technical justification, just wanted to avoid churn if it was purely for "it's not ratified" reasons and we think it'll reappear in another form later Repository: rG LLVM Github Monorepo CHANGES SINCE LAST AC

[PATCH] D111062: [RISCV] Rename some assembler mnemonic and intrinsic functions for RVV 1.0.

jrtc27 added inline comments. Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:1378 +def : InstAlias<"vmornot.mm $vd, $vs2, $vs1", +(VMORN_MM VR:$vd, VR:$vs2, VR:$vs1), 0>; + Ugh, spec mandating aliases for the pre-ratified names... this is ju

[PATCH] D110663: [Driver] Support Debian multiarch style lib/clang/14.0.0/x86_64-linux-gnu runtime path and include/x86_64-linux-gnu/c++/v1 libc++ path

jrtc27 added a comment. In D110663#3029124 , @MaskRay wrote: > In D110663#3029088 , @phosek wrote: > >> The reason I removed this behavior in D101194 >> , aside from extra overhe

[PATCH] D110663: [Driver] Support Debian multiarch style lib/clang/14.0.0/x86_64-linux-gnu runtime path

jrtc27 added a comment. NB: summary has x86_64-known rather than x86_64-unknown in a couple of places Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110663/new/ https://reviews.llvm.org/D110663 ___ cfe-co

[PATCH] D95588: [RISCV] Implement the MC layer support of P extension

jrtc27 added inline comments. Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:219 +def GPR32Pairs : RegisterTuples<[gpr32_pair_lo, gpr32_pair_hi], +[(add X0, X2, X4, X6, + X8, X10, X12, X14, -

[PATCH] D93298: [RISCV] add the MC layer support of Zfinx extension

jrtc27 added inline comments. Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:539 +let RegAltNameIndices = [ABIRegAltName] in { + foreach Index = [0, 2, 4, 6, 8, 10, 12, 14, 16, 18, 20, 22, This needs to be coordinated with D95588; you both define GPR p

[PATCH] D93298: [RISCV] add the MC layer support of Zfinx extension

jrtc27 added a comment. The amount of duplication here really depresses me and is only going to get worse once codegen is added, but TableGen isn't able to have operands that use different register classes based on even HwMode, that I know of, and whilst you could make use of multi classes to g

[PATCH] D95588: [RISCV] Implement the MC layer support of P extension

jrtc27 added inline comments. Comment at: llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:442 + +if (STI.getFeatureBits()[RISCV::FeatureExtZpsfoperand] && +!STI.getFeatureBits()[RISCV::Feature64Bit]) { Jim wrote: > jrtc27 wrote: > > The table

[PATCH] D105516: [clang][PassManager] Add -falways-mem2reg CC1 flag to run mem2reg at -O0

jrtc27 added a comment. @rjmccall Ping? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105516/new/ https://reviews.llvm.org/D105516 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.ll

[PATCH] D101400: [Driver] Add -print-multiarch-triple

jrtc27 added a comment. Answering the last part myself: GCC only has MULTIARCH_DIRNAME definitions for various linux-gnu*, linux-musl, linux-uclibc, kfreebsd-gnu and -gnu (as an OS, for GNU/Hurd) targets, everything else prints a blank string here. So either we need to decide what the right beh

[PATCH] D101400: [Driver] Add -print-multiarch-triple

jrtc27 added a comment. This includes the version number, at least for FreeBSD, in its output, which doesn't make a huge deal of sense to me. This then breaks the build of Python because it generates a module with a name like `_sysconfigdata__freebsd14_x86_64-unknown-freebsd14.0.py`, and when i

[PATCH] D109372: [RISCV][RFC] Add Clang support for RISC-V overlay system

jrtc27 added inline comments. Comment at: clang/include/clang/Basic/Attr.td:1796 +// trigger an error. +def RISCVOverlayCall : InheritableAttr { + let Spellings = [GCC<"overlaycall">]; aaron.ballman wrote: > edward-jones wrote: > > jrtc27 wrote: > > > If you wan

[PATCH] D109372: [RISCV][RFC] Add Clang support for RISC-V overlay system

jrtc27 added a comment. In D109372#2987405 , @MaskRay wrote: > The name "overlay" is ambiguous. Even after I ruled out Gentoo Overlay and > overlayfs, I am thinking whether this has anything to do with `OVERLAY` > description in a linker script: > http

[PATCH] D109372: [RISCV][RFC] Add Clang support for RISC-V overlay system

jrtc27 added a comment. I don't see anything giving an error if you try and use the new badly-named option for an architecture other than RISC-V (beyond the usual unused argument warning that's only an error with -Werror)? Comment at: clang/include/clang/Basic/Attr.td:348 +//

[PATCH] D108886: Add RISC-V sifive-s51 cpu

jrtc27 added a comment. In D108886#2977873 , @apivovarov wrote: > In D108886#2977733 , @jrtc27 wrote: > >> You don't need to tag people as well as adding them as reviewers, it's just >> annoying. Also, it's only

[PATCH] D108886: Add RISC-V sifive-s51 cpu

jrtc27 added a comment. In D108886#2977730 , @apivovarov wrote: > @evandro @kito-cheng @kito.cheng @khchen @MaskRay Could you review this > patch? Thank you You don't need to tag people as well as adding them as reviewers, it's just annoying. Also, it

[PATCH] D108792: [M68k] Update pointer data layout

jrtc27 accepted this revision. jrtc27 added a comment. This revision is now accepted and ready to land. Comments could perhaps be clearer about highlighting that 32-bit integers/pointers are only 16-bit aligned in the ABI as that's the weird bit, not the 32-bit preferred alignment Repository:

[PATCH] D95588: [RISCV] Implement the MC layer support of P extension

jrtc27 added inline comments. Comment at: llvm/lib/Target/RISCV/RISCV.td:186-188 + [FeatureExtZpsfoperand, +FeatureExtZpn, +FeatureExtZprvsfextra]>; Jim wrote: > jrtc27 wrote: > > These aren't

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

jrtc27 added inline comments. Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:49-50 + /// Parse RISCV ISA info from arch string. + static std::unique_ptr + parseArchString(llvm::Error &Error, StringRef Arch, + bool EnableExperimentalExtension, ---

[PATCH] D95588: [RISCV] Implement the MC layer support of P extension

jrtc27 added inline comments. Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:69 +def sub_lo : SubRegIndex<32>; +def sub_hi : SubRegIndex<32, 32>; Jim wrote: > jrtc27 wrote: > > Jim wrote: > > > Jim wrote: > > > > luismarques wrote: > > > > > jrtc27 wrot

[PATCH] D106974: libcang: Add missing function to libclang.map

jrtc27 accepted this revision. jrtc27 added a comment. This revision is now accepted and ready to land. Thanks; I can confirm the test passes if and only if I apply this patch Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106974/new/ https://review

<    1   2   3   4   >