Hi Jocelyn and welcome to the LLVM community. I'm really excited about this approach to testing the assembler. I've just got a few comments for now.

This looks good to me with two caveats

This is looking good to me, I've added some comments. The logic around callee/caller save registers is a little confusing but the way you're handling it seems sensible. A comment clarifying things in determineCalleeSaves would be really useful I think.

Could you add a test that shows .4byte and .long are now correctly handled too? e.g. by extending micromips-label-test.s

Adding Philip Reames as a reviewer, who kindly offered on Twitter to take a look at this.

Sure, but it does sound like the handling of .4byte, .long may incorrectly set STO_MIPS_MICROMIPS in the current implementation so perhaps there's a bug to be filed.

Thanks Ana. I've added some initial review comments. Sorry for the slight delay, I've been on vacation the past week.

Thanks for the review and sorry for the delay in committing. Committed in rL336100.

Hi Ana, if you're planning on updating it would probably be worth following the work on a replacement for SearchableTable:

This was fixed in rL335202. In general, do feel free to direct commit minor test or build fixes without pre-commit review.

Looks good to me, thanks!

Hi Shiva. I think your tests need to demonstrate the behaviour when there is no C extension support as well.

Thanks! Looks good to me, but two minor suggestions:

• arith-with-overflow.ll might be a better name
• Add the nounwind attribute to the function definitions, so we still get clean assembly output when cfi directives are emitted.

This looks good to me (in that the motivation makes sense and the change seems to implement the desired change). Only caveat is that I'm not familiar with the code paths for bundled instructions.

Thanks, this looks good to me assuming the minor nits are addressed. The only one requiring code changes is the for loop in matchLargeOffset.

Thanks, I'll have to finish stepping through the code tomorrow but this seems to be working great. For the GCC torture suite at O1, ~40 of the test cases see improved codegen (fewer instructions). Two gain a single instruction, but that's just noise due to slight regalloc differences.

Updates:

• Expand masked AMOs to IR for address calculation and masking + an intrinsic for the core LL/SC loop. Introduce a new hook in TargetLowering to allow this
• Support for part-word atomicrmw max/min/umax/umin
• Use MaskedMerge rather than calculating an inverse mask
• Expand atomicrmw sub i32 to (AMOADD_W GPR:$addr, (SUB X0, GPR:$incr)) with appropriate AQ/RL bits

Update to add atomicrmw max/min/umax/umin to tests.

In D47882#1125698, @jyknight wrote:

The problem with min and max is that you must not have a branch within the ll/sc loop. But, they can be done branchless with not too many instructions, so ISTM we should just implement them that way.

I think the following will work -- it computes b ^ ((a^b) & -(a < b)). The & -(a < b) either returns the LHS (if comparison is true) or returns 0 (if comparison is false), so you either get b^a^b, or b.

Update to use fence.tso for fence acq_rel.

In D47587#1125518, @asb wrote:

Thanks for querying. I actually had been referring to an earlier draft. But even with the current draft I'm concerned that v2.0 of the ISA manual didn't specify that instr[31:28] should be ignored, meaning fence.tso may simply trap on some implementations. I'll query if anyone is tracking this behaviour across available RISC-V IP cores.

In D47587#1125426, @jyknight wrote:

fence.tso is in the draft spec you linked to -- are you not using it because it hasn't been finalized, or was it just added after you left that comment?

Ping?

Looks good to me. Many thanks for your work on this - I'm committing now.

This patch now enables lowering of 8, 16, and 32-bit atomic load/stores. We no longer rely on D47553, as the follow-up patch to lower atomicrmw supports partword and native size atomics.

Thank you everyone for your comments. I've removed the dependency on this patch from my queue of patches, and instead implement 8/16/32-bit atomics in one go.

Update to address all outstanding review comments.

In D47553#1119463, @efriedma wrote:

MachineOutliner doesn't do anything unless target-specific hooks say a transform is safe; it should be possible to guard against the possibility of outlining an ll/sc pair.

In D47553#1119021, @jyknight wrote:

If you haven't seen it, you might want to read the thread I started a while ago, http://lists.llvm.org/pipermail/llvm-dev/2016-May/099490.html discussing how AtomicExpandPass's feature of generating LL/SC loops at the IR level is a bad idea, and we need a better mechanism in order for it to generate actually-correct code. (We really ought to not expose ll and sc primitives to the IR level at all, because it's impossible to use them correctly).

That fundamental issue hasn't actually been resolved yet -- the current behavior seems to "work well enough" usually. But, I don't know whether the RISCV hardware requires strict adherence to its spec of max 16 base-ISA instructions, except loads, stores, or taken-branches. If so, the generic code in AtomicExpandPass cannot make that guarantee.

In D47553#1118067, @efriedma wrote:

But even if they are lock-free, it still requires linking to the shared library libatomic; we don't want to impose that requirement on users if it isn't necessary.

In D47553#1117786, @jfb wrote:

In D47553#1117763, @asb wrote:

I could understand an argument that use-case 1) isn't compelling enough to add this new hook, and determining whether use-case 2) makes sense is dependent on the conclusion of the GCC bug 86005 discussion. Thanks @jyknight for sharing your thoughts in that GCC thread - much appreciated.

I think you want the ll/sc code for smaller atomics, it's an assumption on many platforms that you have lock-free 8 / 16. / 32 bit atomics, and often double-pointer-wide atomics too. I don't think your libcalls will generally be lock-free. I'd be worried if, even for bringup, your platform at one point in time didn't have that property, because then it sets a precedent for people saying "oh but this one time RISCV didn't do *blah*" and then we'll have endless debates about silly platforms. Please avoid me the debates 😉

Ping?

In D47589#1117978, @asb wrote:

In D47589#1117778, @jfb wrote:

When the A extension is supported, __atomic libcalls will be generated for any atomic that isn't the native word size or has less than natural alignment.

When do you expect non-natural alignment to occur? Is this purely for C++ support? If so you're guaranteed natural alignment. Otherwise (for intrinsics or for other languages) I'd like to understand what you expect, and whether you have the guarantee that the alignment information you have is correct. Without knowing that it's absolutely correct you're going to codegen bad code (a libcall in one place, and instructions in another).

That comment simply reflects the status quo for behaviour of AtomicExpandPass, that I replicated. Do you think it would be worth doing report_fatal_error if Align < Size?

In D47589#1117778, @jfb wrote:

When the A extension is supported, __atomic libcalls will be generated for any atomic that isn't the native word size or has less than natural alignment.

When do you expect non-natural alignment to occur? Is this purely for C++ support? If so you're guaranteed natural alignment. Otherwise (for intrinsics or for other languages) I'd like to understand what you expect, and whether you have the guarantee that the alignment information you have is correct. Without knowing that it's absolutely correct you're going to codegen bad code (a libcall in one place, and instructions in another).

Update to address comments from @jfb (thanks!).

In D47553#1117753, @efriedma wrote:

On other architectures which have 32-bit atomics, but not 8 and 16-bit atomics, it's generally possible to lower smaller operations using a 32-bit cmpxchg loop. Does that not work on RISCV for some reason?

Thanks Ed, looks good to me.

Updated patch to document that shouldExpandAtomicToLibCall should return the same result for objects of the same size.

I'm going to un-abandon this patch. This hook is still helpful in order to e.g. lower to __atomic libcalls for all 8 and 16-bit operations but not 32-bit. This seems acceptable based on the linked GCC and LLVM docs, but isn't possible with the current MaxSizeInBitsSupported. Am I missing any problems with that strategy?

Are there any plans to push this forwards?

Thanks Eli and JF for the rapid feedback. I was too quick to try to replicate GCC behaviour without running sanity checking its output.

Thanks Eli, I actually just came to the same conclusion when taking a step back and thinking about it some more. I was caught up on matching RISC-V GCC behaviour in this case, when in fact it seems RISC-V GCC is broken here. The LLVM docs on atomics also do document this requirement https://llvm.org/docs/Atomics.html#atomics-and-codegen. The 'blunt instrument' of MaxSizeInBitsSupported is acting as designed.

Thanks, looks good to me. Probably worth retitling the commit to e.g. "[RISCV] Support resolving fixup_riscv_call and add to MCFixupKindInfo table"

Ok, there _is_ a problem using addi rather than addiw. Consider loading the immediate 0x7fffffff.

Looks good to me, thanks!

In D44888#1111920, @shiva0217 wrote:

In D44888#1109361, @asb wrote:

This is looking good to me, just needs an update to address this request for a test in riscv-features.c that demonstrates the default +relax/-relax setting.

Hi Alex. I added the testing line on D47127 which is the patch we turn on relaxation as default. Do you think it's ok?

In D45748#1111581, @sabuasal wrote:

Yes. Are we still OK with committing this patch as is? I plan to add a test case that shows the peephole's in ability to handle a GlobalAddress uses at multiple blocks then commit this patch.

After that I'll look into doing all of this in a Machine Function Pass to handle all the corner cases. If it looks good we can revert this patch and keep the pass.

Does that sound like a good plan?

In D45748#1110695, @sabuasal wrote:

In D45748#1110644, @asb wrote:

I'm happy to land this patch as-is. It generates good code in a wide variety of cases (more so than many other backends). But please add the above example to your test file with a note about the missed optimisation opportunity.

The example is actually already added to the test (define dso_local i32 @load_half() nounwind).

@echristo: Do you think you might be able to help review this?

I'd still err towards .option norelax meaning "do what I say". If a user freely intermixes .option relax and .option norelax within a section then there's a whole bunch of things that might break. Simply ignoring the norelax doesn't seem right, and picking out particular cases (e.g. symbol diffs) where we'll act as if relaxation was enabled also seems a bit dodgy. I still think that erroring might be safest and simplest path.

By way of update: the only thing stopping me from committing this right now is convincing myself that there's never a reason to use addiw rather than addi for the first constant when expanding li for RV64. I'm just going to step through some more examples...