In D89288#2338620, @lenary wrote:

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?

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.

As for building correctly:

In D89330#2332401, @Paul-C-Anagnostopoulos wrote:

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?

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

As I said in D88832:

To elaborate:

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.

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).

It was never closed, only accepted.

(and please also close the revision)

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.

In D88832#2319576, @Paul-C-Anagnostopoulos wrote:

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.

In D88832#2318253, @Paul-C-Anagnostopoulos wrote:

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.

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

In D88832#2318159, @Paul-C-Anagnostopoulos wrote:

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.

In D88832#2316928, @Paul-C-Anagnostopoulos wrote:

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

In D88832#2316844, @Paul-C-Anagnostopoulos wrote:

@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>.

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

In D88759#2313236, @thakis wrote:

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?

In D88832#2311870, @Paul-C-Anagnostopoulos wrote:

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.

In D88832#2311859, @Paul-C-Anagnostopoulos wrote:

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

Also please update your review with a full context diff.

Why not make it general and take an array?

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?

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.

In D80465#2283180, @pzheng wrote:

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?

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.

In D79916#2279987, @MaskRay wrote:

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);
}

In D79916#2279901, @Bdragon28 wrote:

In D79916#2279884, @jrtc27 wrote:

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?

In D79916#2279875, @jrtc27 wrote:

In D79916#2279871, @Bdragon28 wrote:

In D79916#2279866, @jrtc27 wrote:

In D79916#2279863, @Bdragon28 wrote:

In D79916#2279816, @jrtc27 wrote:

In D79916#2279812, @Bdragon28 wrote:

In D79916#2279045, @jrtc27 wrote:

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).

In D79916#2279871, @Bdragon28 wrote:

In D79916#2279866, @jrtc27 wrote:

In D79916#2279863, @Bdragon28 wrote:

In D79916#2279816, @jrtc27 wrote:

In D79916#2279812, @Bdragon28 wrote:

In D79916#2279045, @jrtc27 wrote:

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.

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

In D79916#2279863, @Bdragon28 wrote:

In D79916#2279816, @jrtc27 wrote:

In D79916#2279812, @Bdragon28 wrote:

In D79916#2279045, @jrtc27 wrote:

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.

In D82995#2279840, @alanphipps wrote:

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.

In D79916#2279812, @Bdragon28 wrote:

In D79916#2279045, @jrtc27 wrote:

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.

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

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.

In D86522#2278718, @arichardson wrote:

In D86522#2278710, @luismarques wrote:

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.

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.

In D87562#2269791, @vitalybuka wrote:

I prefer we use just "inline"

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)

Needs a test case; something like:

Use llvm-readelf rather than llvm-readobj

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?

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

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

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.

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

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.

Fixed stray non-breaking spaces

In D85366#2211651, @evandro wrote:

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

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.

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

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.

In D85122#2192902, @kschwarz wrote:

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?

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_).

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.

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.

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

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

In D84833#2184858, @luismarques wrote:

In D84833#2184824, @jrtc27 wrote:

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).

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.

In D84833#2184820, @luismarques wrote:

In D84833#2184534, @jrtc27 wrote:

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)

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.