Page MenuHomePhabricator

jrtc27 (Jessica Clarke)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 4 2017, 12:12 PM (197 w, 5 d)

Recent Activity

Today

jrtc27 added a comment to D89288: [RISCV] Enable the use of the old sptbr name.

I think supporting the old CSR names is a small, simple thing that we can do which removes a barrier to people moving to using LLVM rather than GCC. Better this than death by a thousand cuts when people try to move their software over. Yes, people *should* just update their software (and dependencies). The question is will they?

Mon, Oct 19, 6:25 AM · Restricted Project

Sat, Oct 17

jrtc27 added inline comments to D88390: [M68K] (Patch 4/8) MC layer and object file support.
Sat, Oct 17, 11:32 AM · Restricted Project
jrtc27 added inline comments to D88390: [M68K] (Patch 4/8) MC layer and object file support.
Sat, Oct 17, 7:34 AM · Restricted Project
jrtc27 requested changes to D89556: Avoid use of gets() in DOE-ProxyApps-C/Pathfinder.
Sat, Oct 17, 7:17 AM
jrtc27 added inline comments to D88390: [M68K] (Patch 4/8) MC layer and object file support.
Sat, Oct 17, 7:13 AM · Restricted Project

Fri, Oct 16

jrtc27 added a comment to D89557: Fix build of MiBench on FreeBSD RISC-V.

Even if these were implemented on RISC-V they'd just be stubs anyway because there is no support for hardware FPU traps, you have to check fflags in software.

Fri, Oct 16, 8:50 AM

Thu, Oct 15

jrtc27 added a comment to D89330: [RISCV] [TableGen] Modify RISCVCompressInstEmitter.cpp to use getAllDerivedDefinitions().

As for building correctly:

Thu, Oct 15, 8:14 AM · Restricted Project
jrtc27 added a comment to D89330: [RISCV] [TableGen] Modify RISCVCompressInstEmitter.cpp to use getAllDerivedDefinitions().

I do not want the revision to close when I push the patch. I'm using Phabricator to keep track of the status of each patch, so I want to close the revision once I know that the patch built correctly.

Can I cross-link the commit and revision without closing the revision?

Thu, Oct 15, 8:11 AM · Restricted Project
jrtc27 added a comment to D89330: [RISCV] [TableGen] Modify RISCVCompressInstEmitter.cpp to use getAllDerivedDefinitions().

(so please close this revision now and follow the right process in future)

Thu, Oct 15, 8:01 AM · Restricted Project
jrtc27 added a comment to D89330: [RISCV] [TableGen] Modify RISCVCompressInstEmitter.cpp to use getAllDerivedDefinitions().

As I said in D88832:

Thu, Oct 15, 8:00 AM · Restricted Project

Tue, Oct 13

jrtc27 added a comment to D89288: [RISCV] Enable the use of the old sptbr name.

To elaborate:

Tue, Oct 13, 5:08 PM · Restricted Project
jrtc27 added a comment to D89288: [RISCV] Enable the use of the old sptbr name.

Why in 2020 are we *adding* new backwards compatibility aliases? RISC-V is young enough that we don't need to do this, just fix the code. QEMU has even dropped support for 1.9 already.

Tue, Oct 13, 4:59 PM · Restricted Project
jrtc27 accepted D88832: [TableGen] Add new getAllDerivedDefinitionsTwo function to RecordKeeper.

Try now; depending on instance configuration it sometimes only lets you close accepted revisions (based on the assumption that the commit shouldn't have landed unless accepted).

Tue, Oct 13, 1:40 PM · Restricted Project
jrtc27 added a comment to D88832: [TableGen] Add new getAllDerivedDefinitionsTwo function to RecordKeeper.

It was never closed, only accepted.

Tue, Oct 13, 1:24 PM · Restricted Project
jrtc27 added a comment to D88832: [TableGen] Add new getAllDerivedDefinitionsTwo function to RecordKeeper.

(and please also close the revision)

Tue, Oct 13, 11:35 AM · Restricted Project
jrtc27 added a comment to D88832: [TableGen] Add new getAllDerivedDefinitionsTwo function to RecordKeeper.

Please use arc patch next time to get the Differential Revision: header so the commit and the revision are linked in both directions, which will also automatically close the revision.

Tue, Oct 13, 11:35 AM · Restricted Project

Mon, Oct 12

jrtc27 added inline comments to D88832: [TableGen] Add new getAllDerivedDefinitionsTwo function to RecordKeeper.
Mon, Oct 12, 10:54 AM · Restricted Project
jrtc27 requested changes to D88832: [TableGen] Add new getAllDerivedDefinitionsTwo function to RecordKeeper.
Mon, Oct 12, 10:46 AM · Restricted Project

Thu, Oct 8

jrtc27 requested review of D89090: [RISCV] Only return DestSourcePair from isCopyInstrImpl for registers.
Thu, Oct 8, 6:29 PM · Restricted Project
jrtc27 added inline comments to D88832: [TableGen] Add new getAllDerivedDefinitionsTwo function to RecordKeeper.
Thu, Oct 8, 9:48 AM · Restricted Project
jrtc27 added inline comments to D88832: [TableGen] Add new getAllDerivedDefinitionsTwo function to RecordKeeper.
Thu, Oct 8, 9:37 AM · Restricted Project
jrtc27 added inline comments to D88832: [TableGen] Add new getAllDerivedDefinitionsTwo function to RecordKeeper.
Thu, Oct 8, 9:20 AM · Restricted Project
jrtc27 added a comment to D88832: [TableGen] Add new getAllDerivedDefinitionsTwo function to RecordKeeper.

Now the two getAllDerivedDefinitions() functions use StringRef and ArrayRef.

I cleaned up the lints and removed RISCVCompressInstEmitter.cpp from this patch.

I could not use {param} to call the ArrayRef<StringRef> overload; it resulted in an "all paths are infinitely recursive" error.

Thu, Oct 8, 9:11 AM · Restricted Project

Wed, Oct 7

jrtc27 added a comment to D88832: [TableGen] Add new getAllDerivedDefinitionsTwo function to RecordKeeper.

Okay, I think I got it. Thanks for your patience.

The new function takes an ArrayRef<StringRef>. The old function takes a StringRef and simply invokes the new function with makeArrayRef(param).

I had to get used to StringRef and ArrayRef. New to me.

Wed, Oct 7, 4:15 PM · Restricted Project
jrtc27 added a comment to D88832: [TableGen] Add new getAllDerivedDefinitionsTwo function to RecordKeeper.

But yes, this whole time I've wanted a StringRef function and an ArrayRef<StringRef> function, with the former wrapping the latter.

Wed, Oct 7, 3:41 PM · Restricted Project
jrtc27 added a comment to D88832: [TableGen] Add new getAllDerivedDefinitionsTwo function to RecordKeeper.

So why don't I make the new function take ArrayRef<StringRef> and simply restore the original function to its original form. Then there is no problem with the original function accepting a string literal.

Is that a good solution? Otherwise I really don't know what to do.

Wed, Oct 7, 3:40 PM · Restricted Project
jrtc27 added inline comments to D88832: [TableGen] Add new getAllDerivedDefinitionsTwo function to RecordKeeper.
Wed, Oct 7, 3:16 PM · Restricted Project
jrtc27 added a comment to D88832: [TableGen] Add new getAllDerivedDefinitionsTwo function to RecordKeeper.

I tried that, but there is no conversion from a string literal to an ArrayRef<std::string>. Most calls to getAllDerivedDefinitions() pass a literal.

Wed, Oct 7, 8:50 AM · Restricted Project
jrtc27 added a comment to D88832: [TableGen] Add new getAllDerivedDefinitionsTwo function to RecordKeeper.

@jrtc27

It turns out that ArrayRef has a constructor that will convert std::string to ArrayRef<std::string>, making a 1-element array. Therefore, these two overloads are ambiguous when passing a string:

std::vector<Record *>
getAllDerivedDefinitions(const ArrayRef<std::string> Classes) const;

--versus--

std::vector<Record *>
getAllDerivedDefinitions(StringRef ClassName) const;  // The original definition.

I can't think of a solution except to leave the first one as std::vector<std::string> or change it to SmallVectorImpl<std::string>.

Wed, Oct 7, 8:16 AM · Restricted Project

Tue, Oct 6

jrtc27 added a comment to D88832: [TableGen] Add new getAllDerivedDefinitionsTwo function to RecordKeeper.

Use an ArrayRef for the argument and put the one-class overload in a header so it can be inlined?

Tue, Oct 6, 10:45 AM · Restricted Project

Mon, Oct 5

jrtc27 added a comment to D88759: [RISCV] Add SiFive cores to the CPU option.

This seems to break tests: http://45.33.8.238/linux/29545/step_7.txt

Can you take a look and revert for now if it takes a while to fix?

Mon, Oct 5, 5:48 PM · Restricted Project, Restricted Project
jrtc27 added a comment to D88832: [TableGen] Add new getAllDerivedDefinitionsTwo function to RecordKeeper.

Okay, I'm sold. I will overload the existing function with one that takes a set.

I didn't realize that the full context is hidden by default, so when I submit the revised patch I will include full context.

Mon, Oct 5, 7:42 AM · Restricted Project
jrtc27 added a comment to D88832: [TableGen] Add new getAllDerivedDefinitionsTwo function to RecordKeeper.

Is there a way to get full context selectively? I suspect you want full context only on the last two files.

Mon, Oct 5, 7:37 AM · Restricted Project
jrtc27 added a comment to D88832: [TableGen] Add new getAllDerivedDefinitionsTwo function to RecordKeeper.

Also please update your review with a full context diff.

Mon, Oct 5, 6:42 AM · Restricted Project
jrtc27 added a comment to D88832: [TableGen] Add new getAllDerivedDefinitionsTwo function to RecordKeeper.

Why not make it general and take an array?

Mon, Oct 5, 6:41 AM · Restricted Project

Tue, Sep 29

jrtc27 added inline comments to D88389: [M68K] (Patch 3/8) Basic infrastructures and target description files.
Tue, Sep 29, 7:12 PM · Restricted Project
jrtc27 added a comment to D88389: [M68K] (Patch 3/8) Basic infrastructures and target description files.

Why M680x0? Everyone knows it as m68k or 68k and that's what's in the triple, so surely M68K would be the logical target choice?

Tue, Sep 29, 7:11 PM · Restricted Project
jrtc27 added inline comments to D88386: [MIR][M68K] (Patch 2/8): Changes on Target-independent MIR part.
Tue, Sep 29, 7:08 PM · Restricted Project
jrtc27 requested changes to D88385: [TableGen][M68K] (Patch 1/8) Utilities for complex instruction addressing modes: CodeBeads and logical operand helper functions.
Tue, Sep 29, 7:01 PM · Restricted Project

Thu, Sep 24

jrtc27 added inline comments to D88227: [clang-format] Add a SpaceAroundPointerQualifiers style option.
Thu, Sep 24, 7:03 AM · Restricted Project
jrtc27 added inline comments to D88227: [clang-format] Add a SpaceAroundPointerQualifiers style option.
Thu, Sep 24, 7:02 AM · Restricted Project
jrtc27 added inline comments to D88227: [clang-format] Add a SpaceAroundPointerQualifiers style option.
Thu, Sep 24, 6:54 AM · Restricted Project

Tue, Sep 22

jrtc27 added a comment to D87574: [RISCV][ASAN] implementation for vfork interceptor for riscv64.

Can we please just have a riscv version that works for both RV32 and RV64? It should just be a case of changing some ld/sd's to LOAD/STORE macros that are either [ls]w or [ls]d, and the various multiples of 8 to n*__riscv_xlen instead.

Tue, Sep 22, 2:41 PM · Restricted Project

Sep 18 2020

jrtc27 added a comment to D80465: [RISCV-V] Provide muldi3 builtins for riscv.

I noticed that this patch (along with D86036) caused a compiler-rt test to fail for riscv64 when M extension is enabled.

compiler-rt/test/builtins/Unit/muldi3_test.c:11: undefined reference to `__muldi3'

Basically, with this patch, __muldi3 is no longer defined anymore for riscv64 when M extension is enabled. Any suggestion on how this should be fixed?

Sep 18 2020, 4:03 PM · Restricted Project

Sep 17 2020

jrtc27 added a comment to D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness.

If someone cares about pthread_create they might wish to follow up on my https://reviews.llvm.org/D58531, which I filed early last year to permit pthread_create to have a proper type in the syntax. It will likely need rebasing, but probably isn't that much work.

Sep 17 2020, 12:46 PM · Restricted Project
jrtc27 added a comment to D79916: Map -O to -O1 instead of -O2.

Several previous comments are FreeBSD specific. To we clang developers, the concrete request is

Given that GCC will inline at -O, at least these days, ...

right? I think this makes sense, especially when inline is explicitly specified... This appears to be related to some -O1 work @echristo is working on.

// gcc -O1 and g++ -O1 inline `foo`. Note that in C99 mode, `extern int foo` is needed to ask the compiler to provide an external definition.
// clang -O1 and clang++ -O1 do not inline `foo`
inline int foo(int a) {
  return a + a;
}

int bar(int a, int b) {
  return foo(a + b);
}
Sep 17 2020, 12:14 PM · Restricted Project
jrtc27 added a comment to D79916: Map -O to -O1 instead of -O2.

But also you really should not get warnings for unused static functions in included headers, only ones defined in the C source file itself. We'd have countless warnings in the kernel across all architectures otherwise.

I agree. But that's what it is doing when using always_inline in combination with -Wunused-function.

There is currently no real usage of always_inline in system headers though, so maybe I'm just the first to complain about it?

Sep 17 2020, 11:34 AM · Restricted Project
jrtc27 added a comment to D79916: Map -O to -O1 instead of -O2.

This has significantly regressed FreeBSD's performance with the new version of Clang. It seems Clang does not inline functions at -O1, unlike GCC, and since FreeBSD currently compiles its kernel with -O whenever debug symbols are enabled[1] (which, of course, is almost always true), this results in all its static inline helper functions not being inlined at all, a pattern that is common in the kernel, used for things like get_curthread and the atomics implementations.

[1] This is a dubious decision made in r140400 in 2005 to provide "truer debugger stack traces" (well, before then there was ping-ponging between -O and -O2 based on concerns around correctness vs performance, but amd64 is an exception that has always used -O2 since r127180 it seems). Given that GCC will inline at -O, at least these days, the motivation seems to no longer exist, and compiling a kernel at anything other than -O2 (or maybe -O3) seems like a silly thing to do, but nevertheless it's what is currently done.

Cc: @dim @trasz

This is actually SUCH a bad idea that a kernel built with -O will *not work at all* on 32 bit powerpc platforms (presumably due to allocating stack frames in the middle of assembly fragments in the memory management that are supposed to be inlined at all times.) I had to hack kern.pre.mk to rquest -O2 at all times just to get a functioning kernel.

Well, -O0, -O1, -O2 and -O should all produce working kernels, and any cases where they don't are compiler bugs (or kernel bugs if they rely on UB) that should be fixed, not worked around by tweaking the compiler flags in a fragile way until you get the behaviour relied on. Correctness and performance are very different issues here.

As an example:

static __inline void
mtsrin(vm_offset_t va, register_t value)
{

        __asm __volatile ("mtsrin %0,%1; isync" :: "r"(value), "r"(va));
}

This code is used in the mmu when bootstrapping the cpu like so:

for (i = 0; i < 16; i++)
        mtsrin(i << ADDR_SR_SHFT, kernel_pmap->pm_sr[i]);
powerpc_sync();

sdr = (u_int)moea_pteg_table | (moea_pteg_mask >> 10);
__asm __volatile("mtsdr1 %0" :: "r"(sdr));
isync();

tlbia();

During the loop there, we are in the middle of programming the MMU segment registers in real mode, and is supposed to be doing all work out of registers. (and powerpc_sync() and isync() should be expanded to their single assembly instruction, not a function call. The whole point of calling those is that we are in an inconsistent hardware state and need to sync up before continuing execution)

If there isn't a way to force inlining, we will have to change to using preprocessor macros in cpufunc.h.

There is, it's called __attribute__((always_inline)) and supported by both GCC and Clang. But at -O0 you'll still have register allocation to deal with, so really that code is just fundamentally broken and should not be written in C. There is no way for you to guarantee stack spills are not used, it's way out of scope for C.

Is there a way to have always_inline and unused at the same time? I tried using always_inline and it caused warnings in things that used *parts* of cpufunc.h.

Both __attribute__((always_inline)) __attribute__((unused)) and __attribute__((always_inline, unused)) work, but really you should use __always_inline __unused in FreeBSD (which will expand to the former).

Sep 17 2020, 11:23 AM · Restricted Project
jrtc27 added a comment to D79916: Map -O to -O1 instead of -O2.

This has significantly regressed FreeBSD's performance with the new version of Clang. It seems Clang does not inline functions at -O1, unlike GCC, and since FreeBSD currently compiles its kernel with -O whenever debug symbols are enabled[1] (which, of course, is almost always true), this results in all its static inline helper functions not being inlined at all, a pattern that is common in the kernel, used for things like get_curthread and the atomics implementations.

[1] This is a dubious decision made in r140400 in 2005 to provide "truer debugger stack traces" (well, before then there was ping-ponging between -O and -O2 based on concerns around correctness vs performance, but amd64 is an exception that has always used -O2 since r127180 it seems). Given that GCC will inline at -O, at least these days, the motivation seems to no longer exist, and compiling a kernel at anything other than -O2 (or maybe -O3) seems like a silly thing to do, but nevertheless it's what is currently done.

Cc: @dim @trasz

This is actually SUCH a bad idea that a kernel built with -O will *not work at all* on 32 bit powerpc platforms (presumably due to allocating stack frames in the middle of assembly fragments in the memory management that are supposed to be inlined at all times.) I had to hack kern.pre.mk to rquest -O2 at all times just to get a functioning kernel.

Well, -O0, -O1, -O2 and -O should all produce working kernels, and any cases where they don't are compiler bugs (or kernel bugs if they rely on UB) that should be fixed, not worked around by tweaking the compiler flags in a fragile way until you get the behaviour relied on. Correctness and performance are very different issues here.

As an example:

static __inline void
mtsrin(vm_offset_t va, register_t value)
{

        __asm __volatile ("mtsrin %0,%1; isync" :: "r"(value), "r"(va));
}

This code is used in the mmu when bootstrapping the cpu like so:

for (i = 0; i < 16; i++)
        mtsrin(i << ADDR_SR_SHFT, kernel_pmap->pm_sr[i]);
powerpc_sync();

sdr = (u_int)moea_pteg_table | (moea_pteg_mask >> 10);
__asm __volatile("mtsdr1 %0" :: "r"(sdr));
isync();

tlbia();

During the loop there, we are in the middle of programming the MMU segment registers in real mode, and is supposed to be doing all work out of registers. (and powerpc_sync() and isync() should be expanded to their single assembly instruction, not a function call. The whole point of calling those is that we are in an inconsistent hardware state and need to sync up before continuing execution)

If there isn't a way to force inlining, we will have to change to using preprocessor macros in cpufunc.h.

There is, it's called __attribute__((always_inline)) and supported by both GCC and Clang. But at -O0 you'll still have register allocation to deal with, so really that code is just fundamentally broken and should not be written in C. There is no way for you to guarantee stack spills are not used, it's way out of scope for C.

Is there a way to have always_inline and unused at the same time? I tried using always_inline and it caused warnings in things that used *parts* of cpufunc.h.

Sep 17 2020, 11:15 AM · Restricted Project
jrtc27 added a comment to D79916: Map -O to -O1 instead of -O2.

(and FreeBSD has an __always_inline in sys/sys/cdef.s like __inline)

Sep 17 2020, 11:13 AM · Restricted Project
jrtc27 added a comment to D79916: Map -O to -O1 instead of -O2.

This has significantly regressed FreeBSD's performance with the new version of Clang. It seems Clang does not inline functions at -O1, unlike GCC, and since FreeBSD currently compiles its kernel with -O whenever debug symbols are enabled[1] (which, of course, is almost always true), this results in all its static inline helper functions not being inlined at all, a pattern that is common in the kernel, used for things like get_curthread and the atomics implementations.

[1] This is a dubious decision made in r140400 in 2005 to provide "truer debugger stack traces" (well, before then there was ping-ponging between -O and -O2 based on concerns around correctness vs performance, but amd64 is an exception that has always used -O2 since r127180 it seems). Given that GCC will inline at -O, at least these days, the motivation seems to no longer exist, and compiling a kernel at anything other than -O2 (or maybe -O3) seems like a silly thing to do, but nevertheless it's what is currently done.

Cc: @dim @trasz

This is actually SUCH a bad idea that a kernel built with -O will *not work at all* on 32 bit powerpc platforms (presumably due to allocating stack frames in the middle of assembly fragments in the memory management that are supposed to be inlined at all times.) I had to hack kern.pre.mk to rquest -O2 at all times just to get a functioning kernel.

Well, -O0, -O1, -O2 and -O should all produce working kernels, and any cases where they don't are compiler bugs (or kernel bugs if they rely on UB) that should be fixed, not worked around by tweaking the compiler flags in a fragile way until you get the behaviour relied on. Correctness and performance are very different issues here.

As an example:

static __inline void
mtsrin(vm_offset_t va, register_t value)
{

        __asm __volatile ("mtsrin %0,%1; isync" :: "r"(value), "r"(va));
}

This code is used in the mmu when bootstrapping the cpu like so:

for (i = 0; i < 16; i++)
        mtsrin(i << ADDR_SR_SHFT, kernel_pmap->pm_sr[i]);
powerpc_sync();

sdr = (u_int)moea_pteg_table | (moea_pteg_mask >> 10);
__asm __volatile("mtsdr1 %0" :: "r"(sdr));
isync();

tlbia();

During the loop there, we are in the middle of programming the MMU segment registers in real mode, and is supposed to be doing all work out of registers. (and powerpc_sync() and isync() should be expanded to their single assembly instruction, not a function call. The whole point of calling those is that we are in an inconsistent hardware state and need to sync up before continuing execution)

If there isn't a way to force inlining, we will have to change to using preprocessor macros in cpufunc.h.

Sep 17 2020, 11:12 AM · Restricted Project
jrtc27 added a comment to D82995: [UpdateTestChecks] Allow $ in function names.

There exist downstream ARM compilers that doesn't support ARM64 and fail with Inputs/arm_function_name.ll. You should consider guarding arm64 specific tests.

Sep 17 2020, 11:06 AM · Restricted Project
jrtc27 added a comment to D79916: Map -O to -O1 instead of -O2.

This has significantly regressed FreeBSD's performance with the new version of Clang. It seems Clang does not inline functions at -O1, unlike GCC, and since FreeBSD currently compiles its kernel with -O whenever debug symbols are enabled[1] (which, of course, is almost always true), this results in all its static inline helper functions not being inlined at all, a pattern that is common in the kernel, used for things like get_curthread and the atomics implementations.

[1] This is a dubious decision made in r140400 in 2005 to provide "truer debugger stack traces" (well, before then there was ping-ponging between -O and -O2 based on concerns around correctness vs performance, but amd64 is an exception that has always used -O2 since r127180 it seems). Given that GCC will inline at -O, at least these days, the motivation seems to no longer exist, and compiling a kernel at anything other than -O2 (or maybe -O3) seems like a silly thing to do, but nevertheless it's what is currently done.

Cc: @dim @trasz

This is actually SUCH a bad idea that a kernel built with -O will *not work at all* on 32 bit powerpc platforms (presumably due to allocating stack frames in the middle of assembly fragments in the memory management that are supposed to be inlined at all times.) I had to hack kern.pre.mk to rquest -O2 at all times just to get a functioning kernel.

Sep 17 2020, 10:50 AM · Restricted Project
jrtc27 accepted D84414: [RISCV] Support Shadow Call Stack.

Yes I think everything's been addressed now (though if I keep looking over it I might start nit-picking even more :)).

Sep 17 2020, 5:51 AM · Restricted Project, Restricted Project
jrtc27 committed rG788c7d2ec11d: [clang][docs] Fix documentation of -O (authored by jrtc27).
[clang][docs] Fix documentation of -O
Sep 17 2020, 5:47 AM
jrtc27 updated subscribers of D79916: Map -O to -O1 instead of -O2.

This has significantly regressed FreeBSD's performance with the new version of Clang. It seems Clang does not inline functions at -O1, unlike GCC, and since FreeBSD currently compiles its kernel with -O whenever debug symbols are enabled[1] (which, of course, is almost always true), this results in all its static inline helper functions not being inlined at all, a pattern that is common in the kernel, used for things like get_curthread and the atomics implementations.

Sep 17 2020, 5:24 AM · Restricted Project
jrtc27 added a comment to D86522: [RISC-V] Implement RISCVInstrInfo::isCopyInstrImpl().

I was hoping others would chime in regarding what instructions should be handled in this hook.

@arichardson How do you feel about removing the ORI/XORI and landing this patch? (I also have no major objections to just landing as is). We can always further improve it later. Ideally this would add tests, but if that's not easy I think it's OK to land it as is.

Sure, will do that.

Sep 17 2020, 4:59 AM · Restricted Project

Sep 16 2020

jrtc27 added inline comments to D84414: [RISCV] Support Shadow Call Stack.
Sep 16 2020, 4:29 PM · Restricted Project, Restricted Project
jrtc27 added inline comments to D84414: [RISCV] Support Shadow Call Stack.
Sep 16 2020, 11:21 AM · Restricted Project, Restricted Project
jrtc27 requested changes to D84414: [RISCV] Support Shadow Call Stack.

This is currently incompatible with the save/restore libcalls, and I don't think there's any way to avoid that (the restore libcall both loads ra and jumps to it). We should ensure combining them gives an error.

Sep 16 2020, 11:20 AM · Restricted Project, Restricted Project

Sep 13 2020

jrtc27 added inline comments to D87572: [RISCV][ASAN] implementation of internal syscalls wrappers for riscv64.
Sep 13 2020, 12:35 PM · Restricted Project
jrtc27 added a comment to D87562: [compiler-rt] Replace INLINE with inline.

I prefer we use just "inline"

Sep 13 2020, 12:02 PM · Restricted Project

Sep 7 2020

jrtc27 added a comment to D87219: [ELF] Merge .openbsd.randomdata.* sections into a single .openbsd.randomdata section when linking.

Isn't it useless without emitting a PT_OPENBSD_RANDOMIZE to cover it? (reading the somewhat terse https://github.com/openbsd/src/blob/master/libexec/ld.so/SPECS.randomdata)

Sep 7 2020, 9:14 AM · Restricted Project

Sep 6 2020

jrtc27 requested changes to D87210: [Sparc] Remove cast that truncates immediate operands to 32 bits..

Needs a test case; something like:

Sep 6 2020, 2:33 PM · Restricted Project

Sep 5 2020

jrtc27 committed rGbef38e86b4e7: [ELF] Handle SHT_RISCV_ATTRIBUTES similarly to SHT_ARM_ATTRIBUTES (authored by jrtc27).
[ELF] Handle SHT_RISCV_ATTRIBUTES similarly to SHT_ARM_ATTRIBUTES
Sep 5 2020, 10:37 AM
jrtc27 closed D86309: [ELF] Handle SHT_RISCV_ATTRIBUTES similarly to SHT_ARM_ATTRIBUTES.
Sep 5 2020, 10:36 AM · Restricted Project
jrtc27 added inline comments to D86309: [ELF] Handle SHT_RISCV_ATTRIBUTES similarly to SHT_ARM_ATTRIBUTES.
Sep 5 2020, 7:08 AM · Restricted Project
jrtc27 updated the diff for D86309: [ELF] Handle SHT_RISCV_ATTRIBUTES similarly to SHT_ARM_ATTRIBUTES.

Use llvm-readelf rather than llvm-readobj

Sep 5 2020, 7:02 AM · Restricted Project

Sep 4 2020

jrtc27 accepted D86782: [clang-format] Allow configuring list of macros that map to attributes.
Sep 4 2020, 4:36 PM · Restricted Project

Sep 1 2020

jrtc27 requested changes to D86782: [clang-format] Allow configuring list of macros that map to attributes.

The documentation currently shows __capability being included, but from looking at this patch does the configuration file not append (which I think makes sense, at least for __capability) rather than replace?

Sep 1 2020, 7:13 AM · Restricted Project
jrtc27 added a comment to D86621: [clang][Sparc] Default to -mcpu=v9 for SparcV8 on Solaris.

Actually, __sparcv8 is only for V8; if you have 32-bit V9 on Solaris it defines __sparcv8plus _instead_:

Sep 1 2020, 5:13 AM · Restricted Project, Restricted Project

Aug 31 2020

jrtc27 added a comment to D86621: [clang][Sparc] Default to -mcpu=v9 for SparcV8 on Solaris.

And notably _doesn't_ define the V8 macros, which this patch then reintroduces.

Aug 31 2020, 10:59 AM · Restricted Project, Restricted Project
jrtc27 requested changes to D86621: [clang][Sparc] Default to -mcpu=v9 for SparcV8 on Solaris.

GCC on Linux defines __sparc_v9__ even with -m32. I don't know what Solaris does but please don't break other operating systems just because Solaris has broken headers that conflate the CPU and the ABI.

Aug 31 2020, 10:59 AM · Restricted Project, Restricted Project

Aug 25 2020

jrtc27 added a comment to D86292: [LLDB][RISCV] Distinguish between riscv32 and riscv64 based on ELF class.

Not so silly; gdb (well, the names are inherited from bfd) has set architecture riscv:rv32/set architecture riscv:rv64 :)

Aug 25 2020, 1:13 PM · Restricted Project

Aug 24 2020

jrtc27 added a comment to D86480: [RISC-V] ADDI/ORI/XORI x, 0 should be as cheap as a move.

I think ORI and XORI should go together, as they're both moves when the immediate is 0. ADDI with 0 is special as the canonical move instruction, whereas ORI/XORI with 0 are not necessarily moves in microarchitectures, so I don't know whether they should be recognised here or not.

Aug 24 2020, 11:54 AM · Restricted Project

Aug 20 2020

jrtc27 updated the diff for D86309: [ELF] Handle SHT_RISCV_ATTRIBUTES similarly to SHT_ARM_ATTRIBUTES.

Fixed stray non-breaking spaces

Aug 20 2020, 10:44 AM · Restricted Project
jrtc27 added inline comments to D86309: [ELF] Handle SHT_RISCV_ATTRIBUTES similarly to SHT_ARM_ATTRIBUTES.
Aug 20 2020, 10:42 AM · Restricted Project
jrtc27 committed rG3149ec07c024: [RISCV] Enable MCCodeEmitter instruction predicate verifier (authored by jrtc27).
[RISCV] Enable MCCodeEmitter instruction predicate verifier
Aug 20 2020, 10:37 AM
jrtc27 closed D85015: [RISCV] Enable MCCodeEmitter instruction predicate verifier.
Aug 20 2020, 10:37 AM · Restricted Project
jrtc27 requested review of D86309: [ELF] Handle SHT_RISCV_ATTRIBUTES similarly to SHT_ARM_ATTRIBUTES.
Aug 20 2020, 10:35 AM · Restricted Project

Aug 11 2020

jrtc27 added a comment to D85366: [RISCV] Do not mandate scheduling for CSR instructions.

hasSideEffects may imply isNotDuplicable, especially when rematerializing, but the latter prevents duplication more extensively in the middle end (e.g., tail end duplication).

Aug 11 2020, 3:09 PM · Restricted Project

Aug 6 2020

jrtc27 added a comment to D82317: [Clang/Test]: Update tests where `noundef` attribute is necessary.

If changes like this are required for all these tests this is going to be a complete pain for downstream forks like ours. At the very least, make whatever script you used to update these public, as I don't want to have to recreate it from scratch when merging this in. I had enough "fun" with the LLD mass-renaming (UpperCamel -> lowerCamel) and that was _with_ a supposedly-working script (it didn't quite do the right thing and I seem to recall there being two refactoring commits, only one of which had a script); I do not want a repeat of that experience.

Aug 6 2020, 12:35 PM · Restricted Project
jrtc27 accepted D84833: Implement indirect branch generation in position independent code for the RISC-V target.

Subject to the tests all passing, I think this is now good.

Aug 6 2020, 7:15 AM · Restricted Project
jrtc27 requested changes to D84833: Implement indirect branch generation in position independent code for the RISC-V target.
Aug 6 2020, 6:31 AM · Restricted Project
jrtc27 added a comment to D85366: [RISCV] Do not mandate scheduling for CSR instructions.

Have you seen this actually occur? The fact that hasSideEffects is set surely means that it cannot possibly be duplicated, nor scheduled out-of-order with other side-effect-affected instructions? Also, the only CSR instructions that currently get generated _by CodeGen_ are reads of cycle[h], which are very well-behaved. Naive implementations might still take a slow path just because they're CSR instructions, but it'd be very easy microarchitecturally to quickly read the cycle counter in an ALU as if it were just a normal GPR move. In such cases you really would want to be able to express scheduling information for it just like other arithmetic operations.

Aug 6 2020, 5:59 AM · Restricted Project

Aug 5 2020

jrtc27 updated subscribers of D85122: [GlobalISel][InlineAsm] Fix matching input constraint to physreg.

Either you can create a bug in Bugzilla (https://bugs.llvm.org/), asking for this revision to be merged to the release branch and mark it as a release blocker of the 11.0.0 release, or you can contact the release manager directly (hans@chromium.org).

@jrtc27, does this fix the issue you are seeing in sel4?

Aug 5 2020, 8:53 PM · Restricted Project
jrtc27 accepted D85067: [RISCV] Enable the use of the old mucounteren name.

Looks fine, other than please mention something about it being an alias in the commit subject when landing as currently it reads as if it's a new CSR (and I'd also personally change "This patch enables the use of mucounteren." to "This patch enables the use of the old mucounteren name." to make it completely clear, but if the subject is fixed then I don't think it's _necessary_).

Aug 5 2020, 6:06 AM · Restricted Project

Aug 2 2020

jrtc27 added a comment to D85067: [RISCV] Enable the use of the old mucounteren name.

Patch looks good but please fix the title and description of this revision. It's not about adding AltName, it's about adding mountinhibit itself, and as part of that you're including support for the older name too.

Aug 2 2020, 8:27 AM · Restricted Project

Aug 1 2020

jrtc27 added a comment to D83384: [GlobalISel][InlineAsm] Fix buildCopy for inputs.

Actually the code in the backtrace points at D82651, not this. I did however see buildAnyextOrCopy in a backtrace when playing around to get that minimal case, which is what brought me to this revision. Investigating further, it seems that this is because of D82651 not fully implementing this case, and future revisions not fixing that. Prior to that revision, inline asm would hit reportTranslationError and then fall back to SelectionDAG due to isGlobalISelAbortEnabled being true (provided -global-isel wasn't passed to override that, as is done in my test, but not the original C), but now, since it claims to be implemented but not correctly, it instead asserts deep down and is unable to fall back to SelectionDAG.

Aug 1 2020, 1:37 PM · Restricted Project
jrtc27 updated subscribers of D83384: [GlobalISel][InlineAsm] Fix buildCopy for inputs.

I believe this has broken AArch64 (-global-isel not required due to defaults, but it's a GlobalISel bug so should force it nonetheless):

Aug 1 2020, 11:20 AM · Restricted Project
jrtc27 added inline comments to D85067: [RISCV] Enable the use of the old mucounteren name.
Aug 1 2020, 4:45 AM · Restricted Project
jrtc27 requested changes to D85067: [RISCV] Enable the use of the old mucounteren name.
Aug 1 2020, 4:43 AM · Restricted Project

Jul 31 2020

jrtc27 requested review of D85015: [RISCV] Enable MCCodeEmitter instruction predicate verifier.
Jul 31 2020, 3:15 AM · Restricted Project

Jul 30 2020

jrtc27 added inline comments to D84833: Implement indirect branch generation in position independent code for the RISC-V target.
Jul 30 2020, 8:17 PM · Restricted Project
jrtc27 added a comment to D84414: [RISCV] Support Shadow Call Stack.

There is a possibly-compelling argument against using x18: RV32E only gives x0-x15, so would not be able to support the current implementation.

Jul 30 2020, 5:00 PM · Restricted Project, Restricted Project
jrtc27 added a comment to D84833: Implement indirect branch generation in position independent code for the RISC-V target.

I _believe_ we need:

isBranch = 1, isTerminator = 1, isBarrier = 1

?

Sounds about right. (I don't know offhand in what circumstances you have different values for isTerminator and isBarrier).

Jul 30 2020, 7:57 AM · Restricted Project
jrtc27 requested changes to D84833: Implement indirect branch generation in position independent code for the RISC-V target.

Given @luismarques's comment, have you now actually run the tests (and I mean all RISC-V tests, not just branch-relaxation.ll, in case anything has been missed)? Perhaps I shouldn't take it for granted that people run tests before submitting patches, including revisions (though I'd still verify myself before committing anything), but you really should as otherwise it just wastes our time having to tell you they're broken when you could just run them yourself.

Jul 30 2020, 7:42 AM · Restricted Project
jrtc27 added a comment to D84833: Implement indirect branch generation in position independent code for the RISC-V target.

Other than the TODO, yes (and hopefully you agree with my suggestions here!). I would also like to see branch-relaxation.ll grow RV64 RUN lines for completeness, even if it's probably a bit redundant (and creates a slight explosion..), but I can do that as a follow-up patch.

This patch still seems to need some additional work. When I apply it and run the tests I get a crash for the branch-relaxation.ll test:

*** Bad machine code: MBB has unexpected successors which are not branch targets, fallthrough, EHPads, or inlineasm_br targets. ***
- function:    relax_jal
- basic block: %bb.3  (0x5621c4161a48)
Jul 30 2020, 7:29 AM · Restricted Project
jrtc27 added a comment to D84833: Implement indirect branch generation in position independent code for the RISC-V target.
In D84833#2184491, @asb wrote:

@jrtc27 are you happy with this patch now? Thanks for your review on this and thanks @msizanoen1 for providing this fix.

I think @jrtc27's suggestion about deleting/moving the TODO line in test code still needs to be addressed.

Jul 30 2020, 5:40 AM · Restricted Project

Jul 29 2020

jrtc27 added inline comments to D84833: Implement indirect branch generation in position independent code for the RISC-V target.
Jul 29 2020, 8:01 AM · Restricted Project