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

You've lost the check for the non-PIC one and didn't check that the PIC one actually assembles (it really should, but you never know; the point is that unless you emit a .o the fixups are never actually computed and thus you won't get errors if they end up overflowing).

(and my ; TODO: Extend simm12's MCOperandPredicate so the jalr zero is printed as a jr. then goes away, not because that's been fixed, but because by using PseudoJump we no longer expose it... perhaps that comment should be relocated to RISCVInstrInfo.td's definition of simm12, but it's a minor stylistic thing, it doesn't really matter beyond not being the canonical alias)

This looks good to me now, let's see what others think. But please update llvm/test/CodeGen/RISCV/branch-relaxation.ll to reflect the change in CodeGen from LUI/JALR to AUIPC/JALR, and add PIC RUN lines. Plus whilst you're there I guess it wouldn't hurt to add RV64 variants too.

In D84833#2181555, @msizanoen1 wrote:

This still need to be backported to rust-lang/llvm-project fork which is still on LLVM 10 so using PseudoJump is not an option.

Although I feel PseudoJump might be a sensible choice even for position-dependent code? By this point we know the branch target is more than 2 MiB away, and I assume that neither LUI/JALR nor AUIPC/JALR will be either relaxable at link time (branch target would need to be a low address) or compressible. Plus AUIPC/JALR is more like JAL in being PC-relative, so it "feels" more "faithful".

Please avoid duplication. Something like this should be better in that regard, both for duplicating between the if/else branches and duplicating with the existing AUIPC-based expansions elsewhere.

(the fact that FreeBSD kernel builds could trigger relaxation is what exposed the bug fixed in https://reviews.llvm.org/D77443)

In D82239#2124411, @vb000 wrote:

Hi @jrtc27,

fdiv and fsqrt are rarely used instructions for many applications, but are beasts in terms of HW area consumption. While most of the F extension needs incremental additions to core FPU, fdiv and fsqrt need separate blocks of HW. Often this special purpose HW (whose sole purpose is to support two rarely used instructions) is not worth it in embedded applications. So, it's often fine to emulate those two instructions. So, there's definite value in having this option.

(Also, this is not implemented just because GCC has this. We need this option to use LLVM on an already taped-out chip. I'm just trying to make it easy to transition from GCC to LLVM for ourselves, as well for other who might find this useful.)

1. Please use local variables with meaningful names for RISCV::Xn rather than inlining them everywhere and making it harder at a glance to work out what's going on without knowing what the ABI register names are.

Is there a reason for choosing X18? On AArch64 that's either a temporary or saved register depending on ABI, but determined to be the "platform register". Picking X18 on RISC-V because that's the same index as AArch64 seems a little arbitrary, but maybe it happens to make sense.

In D83082#2132040, @sbc100 wrote:

In D83082#2132017, @jrtc27 wrote:

In D83082#2131971, @sbc100 wrote:

This change seems to have broken one of our tests in emscripten. I reduced it to this:

#include <stdlib.h>
#include <assert.h>

int main() {
  void * ptr = aligned_alloc(3, 64);
  assert(ptr == NULL);
  return 0;
}

Yeah, this is testing DR 460's new (and accepted) requirements (previously it was undefined):

If the value of alignment is not a valid alignment supported by the implementation or the value of size is not an integral multiple of alignment the function shall fail by returning a null pointer.

Regardless of weather its defined or undefined behaviour I wouldn't expect the compiler to crash though right? Clearly this is a compiler bug, right?

In D83082#2131971, @sbc100 wrote:

This change seems to have broken one of our tests in emscripten. I reduced it to this:

#include <stdlib.h>
#include <assert.h>

int main() {
  void * ptr = aligned_alloc(3, 64);
  assert(ptr == NULL);
  return 0;
}

My question is: why do we want this additional complexity? F and D both require FMUL/FDIV to be implemented, so saying you support F (and D) but no FMUL/FDIV is a contradiction, no such implementation can possibly exist (it would instead be say RV32I plus a non-standard extension that looks like a subset of the floating-point one). If you want to optimise for area, don't include an FPU, and if you want speed, include one, but this seems like a strange halfway house. Just because GCC has an option for something doesn't mean it makes sense for us to copy. What is your actual use case for this? Are there RISC-V implementations in the wild that do this and we're missing out on being able to target them, or is this just a hypothetical?

In D77443#2118980, @lewis-revill wrote:

Bumping this patch - Looking at the output of the riscv-expand-pseudos pass I'd say these values are accurate. I'd say comments here, and in the functions which expand the instructions to ensure things don't get changed - seems to me like tests would be a little tricky so perhaps omitting that would be fine just to get this landed.

Yeah, I don't think we want to be merging code we can't test even in a non-automated way. Even if this code is completely bug-free, the inability to test it just means we risk having it bit-rot with nobody noticing.

Macro-op fusion likely means that we'll always schedule the AUIPC with its consumer anyway, but in theory there could be pipelines that don't macro-op fuse and stall on RAW hazards, which could benefit from splitting the pair up.

In D78545#2030910, @Jim wrote:

In D78545#2030863, @jrtc27 wrote:

I am still very confused why this is necessary. CanLowerReturn barely does anything other have a loop to call other functions that actually do useful work, it seems like the wrong place to hook into things. Your "explanation" is merely just "we want this because we want this". Could you please shed some light on how _exactly_ this is useful? As it stands I am none the wiser.

In CanLowerReturn, it calls CC_RISCV in a loop to check if the type can be returned directly. Actually, the change should be in CC_RISCV if we want a specific type returned indirectly.
But this change is maintained hard for downstream while we upgrade or merge the code from trunk. Let CanLowerReturn public/protected for overridden and reusing by the derived class without touching CC_RISCV.

I am still very confused why this is necessary. CanLowerReturn barely does anything other have a loop to call other functions that actually do useful work, it seems like the wrong place to hook into things. Your "explanation" is merely just "we want this because we want this". Could you please shed some light on how _exactly_ this is useful? As it stands I am none the wiser.

Having createFPImm and createSFPImm (similarly for the add methods) seems confusing. Can we not have a uniform SFP/DFP split in the names everywhere, since you're already touching the uses of the former due to changing the types?

I'm curious as to whether this is actually useful. At least for our CHERI fork, we are tightly integrated with the existing code, reusing bits here and there and hooking in where we need to override things. Are you really able to implement any meaningful extensions by overriding the functions without simply duplicating a lot of the logic (which, whilst it avoids merge conflicts, means that any changes and bug fixes likely don't get copied to your forked functions)?

I'd argue more of the comments than those highlighted are also pointless. There's no point writing a comment spelling out what the next line of code clearly does. They should be reserved for overviews of what's going on, and the details of why the code is as it is. I'm also curious as to why you went for the optimisation relaxations rather than R_RISCV_ALIGN. Support for the latter is mandatory for correctness when using -mrelax, but the former is purely optional. Therefore, I would strongly argue that support for R_RISCV_ALIGN should be added before or in the same commit as any R_RISCV_RELAX optimisations, otherwise we're just shipping something that's knowingly broken.

In D77672#1968560, @ruiu wrote:

Improving SPARC support is fine as long as we don't need to do something special for the ISA. But since no active maintainers have a SPARC machine, it is hard to find bugs in SPARC. Can you add tests for the new relocations?

In D77672#1967700, @MaskRay wrote:

My immediate reaction is "isn't Sparc an abandoned architecture?" See https://en.wikipedia.org/wiki/SPARC and https://lists.freebsd.org/pipermail/freebsd-sparc64/2020-January/010192.html

Though, no objection. You may still need some basic tests, similar to test/ELF/ppc32-* I tried hard to keep the number of tests small yet complete. Does sparc64 use TLS variant 2?

Just as a note for things that are still missing:

Correct comment.

In D74453#1931264, @luismarques wrote:

Hey @jrtc27 . Is there anything blocking this patch being committed?

In D75729#1917804, @jrtc27 wrote:

In D75729#1916662, @luismarques wrote:

In D75729#1912792, @rogfer01 wrote:

Perhaps these are slightly better than just fcvt everywhere because they avoid the rounding mode required by fcvt.s.w / fcvt.s.l / fcvt.d.l?

With the rounding mode being stateless (it's per instruction), I'm don't know what the issue would be with that.

Unless you fast-path register zero (or the value itself being zero), a naive implementation is going to have to do a full integer->float conversion, which is more likely to be a multi-cycle operation, regardless of rounding mode. Contrast that with an fmv, which is a special case of fsgnj, but the generic case is still just bit selection and concatenation, so it should always be single-cycle. Having said that, the Rocket schedule (and reading the code) indicates that both are 2-cycle operations, and for the Bluespec Piccolo/Flute cores both are 1-cycle operations, but the GCC schedule for the 7 series SiFive cores claims that fmv will be in the A pipe (address, i.e. loads/stores, but also any FP<->int given that already needs to be present on the load/store paths), and fcvt will be in the B pipe (branches, but also mul/div and and any other FP ops).

In D75729#1916662, @luismarques wrote:

In D75729#1912792, @rogfer01 wrote:

Perhaps these are slightly better than just fcvt everywhere because they avoid the rounding mode required by fcvt.s.w / fcvt.s.l / fcvt.d.l?

With the rounding mode being stateless (it's per instruction), I'm don't know what the issue would be with that.

I believe my previous comments have indeed been addressed.

In D74704#1878921, @lenary wrote:

I am worried about the interaction between -fuse-ld=lld and linker relaxation (which is not supported by LLD, as I understand it)

1. clang could ignore -fuse-ld=lld when linker relaxation is enabled
2. clang could ignore (disable) linker relaxation if -fuse-ld=lld is used

At the very least, a warning of some kind should be emitted if linker relaxation is combined with -fuse-ld=lld.

In D74453#1872395, @lenary wrote:

Is there a reason this hasn't caused code changes for i16 or i8?

Ping?

Removed atomic-rmw.ll change; nothing actually changed in there other than comments and a missing . on labels, so that churn can be postponed until it actually gets touched in a meaningful way.

Indexed load and store fusion should not be exclusive to LD and SD. It applies to any (F)Lx/(F)Sx. The literature just talks about LD/ST (note that this is not SD) in the sense of an arbitrary load/store instruction. Similarly for the various other fusion pairs involving memory accesses.

Switch from explicit iterator to recently-added range-based for loop interface. Ok to merge?

In D73170#1833793, @jhibbits wrote:

Thanks for doing this! It's been on my TODO list for a little while already for SPE, but I never got around to it.

Is there any provision for spilling only the subreg, not the superreg? It seems to me that whenever the subreg needs spilled, it always also spills the superreg, even if the full superreg is not used.

Pass correct target to shouldForceRelocation (although unused).

Addressed review comments.

In D73211#1834739, @efriedma wrote:

This approach seems much easier to understand.

Can you add a testcase for the case where the auipc and addi are in different fragments, like I mentioned in D71978?

Never mind, I see why this is done, we need to respect the ordering, so skipping the SC misses the release.

In D71820#1819146, @jrtc27 wrote:

@MaskRay Do you think we could get this landed (with or without D71822) before 10 branches tomorrow?

Rebased and addressed review comments.