[PATCH] D148817: [RISCV] Add Tag_RISCV_arch attribute by default when using clang as an assembler.

jrtc27 added a comment. Surprised that we didn't already do this; seems like a bit of an oversight. Comment at: clang/test/Driver/riscv-default-build-attributes.s:1 +// Enabled by default for assembly +// RUN: %clang --target=riscv64 -### %s 2>&1 \ Do we use //

[PATCH] D148817: [RISCV] Add Tag_RISCV_arch attribute by default when using clang as an assembler.

jrtc27 added a comment. Some more... hopefully spotted everything this time, sorry Comment at: clang/test/Driver/riscv-default-build-attributes.s:1 + Enabled by default for assembly +// RUN: %clang --target=riscv64 -### %s 2>&1 \ Comment

[PATCH] D148817: [RISCV] Add Tag_RISCV_arch attribute by default when using clang as an assembler.

jrtc27 added inline comments. Comment at: clang/test/Driver/riscv-default-build-attributes.s:3 +// RUN: %clang --target=riscv64 -### %s 2>&1 \ +// RUN:| FileCheck %s -check-prefix CHECK-ENABLED + craig.topper wrote: > jrtc27 wrote: > > jrtc27 wrote: > > > `=`

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

jrtc27 requested changes to this revision. jrtc27 added a comment. This revision now requires changes to proceed. Clang tests should not compile to asm. You want an IR test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153008/new/ https://reviews.

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

jrtc27 added a comment. In D153008#4425238 , @abel-bernabeu wrote: > In D153008#4424821 , @jrtc27 wrote: > >> Clang tests should not compile to asm. You want an IR test. > > Jessica, are there any exceptions for

[PATCH] D138722: Overload all llvm.annotation intrinsics for globals argument

jrtc27 added inline comments. Comment at: clang/test/CodeGen/annotations-field.c:5 -// CHECK: private unnamed_addr constant [8 x i8] c"v_ann_{{.}}\00", section "llvm.metadata" -// CHECK: private unnamed_addr constant [8 x i8] c"v_ann_{{.}}\00", section "llvm.metadata" +// CHE

[PATCH] D139302: [RISCV] Add Syntacore SCR1 CPU model

jrtc27 added inline comments. Comment at: llvm/include/llvm/Support/RISCVTargetParser.def:22 PROC(SIFIVE_U74, {"sifive-u74"}, FK_64BIT, {"rv64gc"}) +PROC(SCR1_BASE, {"scr1-base"}, FK_NONE, {"rv32ic"}) +PROC(SCR1_MAX, {"scr1-max"}, FK_NONE, {"rv32imc"}) Alphabeti

[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] 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] 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] 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] 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] 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] 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] 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] 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] 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#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] 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] 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] 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] 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 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] 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] D101462: Make it possible for targets to define their own MCObjectFileInfo

jrtc27 added a comment. Herald added a subscriber: JDevlieghere. What's the benefit of less-aligned functions? Is that not more likely to get poor performance due to cache line straddling of the first instruction? Regardless, the functional change should be separated out from the refactoring.

[PATCH] D101352: [DOCS] Removed inconsistency in clang vs Clang usage in docs (c vs C)

jrtc27 added a comment. I'm not convinced by these changes? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101352/new/ https://reviews.llvm.org/D101352 ___ cfe-commits mailing list cfe-commits@lists.llvm.

[PATCH] D101462: [MC] Untangle MCContext and MCObjectFileInfo

jrtc27 added a comment. In D101462#2739404 , @MaskRay wrote: > In D101462#2733726 , @flip1995 > wrote: > >>> I'll keep this open for a few days as it touches too many things. >> >> Sounds good 👍 >> >>> but you'll

[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. 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 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] 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] 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] 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] 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] 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] 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] 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 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] D98113: [Driver] Also search FilePaths for compiler-rt before falling back

jrtc27 added a comment. In D98113#2634758 , @luismarques wrote: > I just looked at this again and I don't have the full context in my mind > right now but won't the test just exercise the BareMetal toolchain and not > your changes? I've since lost my re

[PATCH] D98881: [RISCV] Fix mcount name for Linux

jrtc27 added a comment. This should really be for all OSes on RISC-V, which I think means copying it N times? :/ (GCC defines MCOUNT_NAME in riscv/riscv.h, not riscv/linux.h) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98881/new/ https://review

[PATCH] D99009: [RISCV] [1/2] Add intrinsic for Zbr extension

jrtc27 added a comment. Please fix the style issues and update the patch with full context (see https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface, or use `arc`) Comment at: clang/include/clang/Basic/BuiltinsRISCV.def:20 +// zbr extension +TARG

[PATCH] D99009: [RISCV] [1/2] Add intrinsic for Zbr extension

jrtc27 added inline comments. Comment at: llvm/test/CodeGen/RISCV/rv32Zbr.ll:1 +; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py +; RUN: llc -mtriple=riscv32 -mattr=experimental-zbr -verify-machineinstrs < %s \ craig.topper wrote: >

[PATCH] D99082: [RISCV][NFC] Fix RVV intrinsic tests.

jrtc27 added a comment. Most likely because you're adding assembly tests in Clang, which won't work if the backend isn't present (and needs a REQUIRES line). Assembly tests in Clang are generally bad practice and best avoided for that reason. If you do want them it's probably best to split them

[PATCH] D99158: [RISCV][WIP] Implement intrinsics for P extension

jrtc27 added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11170-11175 // RISC-V V-extension def err_riscvv_builtin_requires_v : Error< "builtin requires 'V' extension support to be enabled">; +// RISC-V P-extension +def err_riscvv_builtin_re

[PATCH] D99151: [RISCV][Clang] Add RVV vleff intrinsic functions.

jrtc27 added inline comments. Comment at: clang/test/CodeGen/RISCV/rvv-intrinsics/vle16ff.c:9 +// RUN: -target-feature +experimental-zfh -target-feature +m -fallow-half-arguments-and-returns -Werror -Wall -S -o - %s >/dev/null 2>%t +// RUN: FileCheck --check-prefix=ASM --allow

[PATCH] D99151: [RISCV][Clang] Add RVV vleff intrinsic functions.

jrtc27 added inline comments. Comment at: clang/test/CodeGen/RISCV/rvv-intrinsics/vle16ff.c:9 +// RUN: -target-feature +experimental-zfh -target-feature +m -fallow-half-arguments-and-returns -Werror -Wall -S -o - %s >/dev/null 2>%t +// RUN: FileCheck --check-prefix=ASM --allow

[PATCH] D99151: [RISCV][Clang] Add RVV vleff intrinsic functions.

jrtc27 added inline comments. Comment at: clang/test/CodeGen/RISCV/rvv-intrinsics/vle16ff.c:9 +// RUN: -target-feature +experimental-zfh -target-feature +m -fallow-half-arguments-and-returns -Werror -Wall -S -o - %s >/dev/null 2>%t +// RUN: FileCheck --check-prefix=ASM --allow

[PATCH] D99009: [RISCV] [1/2] Add intrinsic for Zbr extension

jrtc27 added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11190 +def err_riscv_builtin_requires_extension : Error< + "builtin requires %0 extension support to be enabled">; } // end of sema component. This lost the quotes

[PATCH] D99009: [RISCV] [1/2] Add intrinsic for Zbr extension

jrtc27 added a comment. Thanks, and good point re ordering Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99009/new/ https://reviews.llvm.org/D99009 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D99009: [RISCV] [1/2] Add intrinsic for Zbr extension

jrtc27 added a comment. In D99009#2666793 , @thakis wrote: > Looks like this doesn't build on windows: > http://45.33.8.238/win/36271/step_4.txt > > Please take a look and revert for now if it takes a while to fix. Hopefully fixed in 1bd4986e7cdc

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

jrtc27 added a comment. Your tests look like copies of the F/D/Zfh tests with not all the comments updated and instances of tests that just don't make sense for Zfinx. I only skimmed them and picked up a few issues, I haven't gone through them thoroughly, please do that yourself. ===

[PATCH] D70401: [WIP][RISCV] Implement ilp32e ABI

jrtc27 added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:10323 + bool EABI) + : DefaultABIInfo(CGT), XLen(XLen), FLen(FLen) { +if (EABI) I think it'd be better to have a `NumArgGPRs(EAABI ? 6 : 8)` here as having a defaul

[PATCH] D95016: [Clang][RISCV] Add custom TableGen backend for riscv-vector intrinsics.

jrtc27 added inline comments. Comment at: clang/include/clang/Basic/riscv_vector.td:9 +// +// This file defines builtins for RISCV-V V-extension. See: +// "the" Comment at: clang/include/clang/Basic/riscv_vector.td:23 +// The elements of this c

[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] 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: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] 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] 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] 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] 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] 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] 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] 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 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] 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] 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

<    1   2   3   4