In D46423#1243098, @lewis-revill wrote:

Just an update to my previous comment, I realised you were right @asb since we don't update the AsmBackend's SubtargetInfo anymore, so getFeatureBits()[RISCV::FeatureRelax] will still be true after parsing based on the -mattr=relax. I'm looking into potentially using only the ForceRelocs variable for relocation behaviour and trying to set this lazily when we parse an instruction that might be relaxed in a relaxable section to cover the case you mentioned. I'll update whether I think this is possible ASAP.

Thanks Ana, it was indeed intentional that this patch allows li a0, %lo(1234) but the test coverage was lacking. Updated to improve tests.

Update to consistently use loophead/looptail labels in comments.

Thanks for the updates Lewis. I think this is a good solution to the problem. I need to step through the code in more detail to give a final LGTM, but I think it resolves our questions.

Update to add a 'However' has suggested by @jyknight. Thanks for the review.

Re-abandoning.

Closed by rL342487

In D48131#1131431, @efriedma wrote:

You can introduce a target-specific SelectionDAG node without adding a corresponding IR intrinsic. See ISD::FIRST_TARGET_MEMORY_OPCODE.

Updated so common code for generating masks etc is reused through AtomicExpandPass.

In D48130#1132313, @t.p.northover wrote:

It seems a bit of a shame to hide that masked expansion away in lib/Target/RISCV when MIPS (at least) needs similar logic. I suppose someone who wants to refactor MIPS can move it out again though.

I've changed this to better match the revised approach taken for atomicrmw expansion, which maximises the amount of code that can be shared between targets. See D48131 to see this interface put to use.

The patch has been updated so sign-extension is performed correct for part-word signed min/max operations.

Thanks, looks good to me. Note that the IsValid local variables can now be removed.

Looking at the current spec draft, it's not clear that a zero fence argument is actually invalid (though it may be non-sensical and impossible to generate from the assembler). I note that gas will emit "unknown" for the operand when disassembling. I'm not fully sure what the best solution is here, so first wanted to check if there was part of the spec I might have missed that discussed all-zero predecessor/successor fence arguments.

Thanks, looks good to me.

Looks good to me, thanks. Just one tiny comment inline.

Looks good to me, thanks.

I've been trying to build this locally. The cmake/modules/ProtobufMutator.cmake in Clang defines GIT_REPOSITORY and GIT_TAG, enabling protobuf to be checked out and built automatically. Is there a reason to not try to support that here? It seems to work for me when I edit to set GIT_REPOSITORY in the came way as the version of this file in Clang. I do need to go and manually add a symlink from lib to lib64 in /tools/llvm-mc-assembly-fuzzer/protobuf_mutator/src/protobuf_mutator-build/external, but I had that seem issue with the clang protobuf fuzzer build process.

Looks good to me, thanks!

In D50790#1213227, @apazos wrote:

Alex, I did not understand your comment.
"jal a3" parsing will crash due to ErrorInfo == Operands.size(), which this patch is fixing.

Really good catch - thanks for the fix!

Actually, hold-off on reviewing. I don't think the logic for sign extension is correct when the target memory location isn't 32-bit aligned. Sorry for the noise. I'll add look again and add more tests to cover this.

In fact, I'm now wondering if .option rvc/norvc fails to adjust the nop behaviour because MCAsmBackend doesn't get access to an updated STI.

Actually, how about replacing those two lines with auto LastFrameDestroy = std::prev(MBBI, MFI.getCalleeSavedInfo().size())?

Update to sign-extends the values for signed min/max, which is the last known issue with this patch.

Thanks for the patch and sorry for the delay. Once someone else marked it accepted it moves to a separate list in Phabricator - obviously I need to check the 'waiting on authors' list better.

Many thanks for the update.

Looks good to me, thanks. Great to see the fuzzer uncovering bugs like this.

Thanks, looks good to me.

This patch doesn't really fix the problem with jal a3 though catching these target-specific invalid operand errors seems worth doing.

Thanks, looks good to me.

It would also be worth testing the treatment of alignment directives emitted when generating a constant pool. Currently ConstantPool::emitEntries will use EmitCodeAlignment, but D45961 suggests changing it to EmitCodeAlignment. This affects whether EmitNops is true for the MCAlignFragment. However this patch doesn't query that property anyway. It might be worth documenting that limitation and perhaps double-checking GCC behaviour. I assume that if you use .align with a value to fill with, GCC ignores that value if linker relaxation is enabled?

Thanks, looks good to me. This function was rendered unused in rL330827.

My main concern with this patch currently is the ability to handle the relax option changing due to .option relax/norelax. So it's probably blocked on D46423.

I like the idea of rejigging this so getLastSubtargetInfo isn't needed.

Rebase on top of D48129 and update to address most review comments.

Thanks Ana, some initial thoughts:

I've updated this patch so it is independent of the RISC-V atomics lowering. I replace the previous logic in performMaskedAtomicOp with a new helper function widenPartwordAtomicRMW, which is called early on. See the updated summary for more details.

Do you think you might have a chance to update this patch?

Are you planning on updating this? I still feel that either producing an error, or doing what the user says and honouring .option relax/norelax is the best option. Having .option relax 'sticky' for symbol differences slightly reduces the amount of broken code if a user switches between relax/norelax in the same section, but you still have the potential for calls or branches being resolved incorrectly don't you?

You will also need to patch at least clang tools/driver/cc1as_main.cpp due to the change to InitSections.

Other than two tiny formatting issues, this looks good to me.

The main update in this patch revision is moving more of the IR generation for the masked atomic operation to AtomicExpandPass, reusing the createMaskInstrs helper used by expandPartWordAtomicRMW and expandPartWordCmpXchg. We now introduce AtomicExpansionKind::MaskedIntrinsic rather than AtomicExpansionKind::Custom.

Thanks Kito, looks good to me.

Thanks Kito, this looks good to me. Some minor spelling issues in the comment.

Thanks, this looks good to me.

In D50164#1186326, @jocewei wrote:

In D50164#1185810, @asb wrote:

Hi Jocelyn. What should I be referring to as the base of this patch? e.g. D49521 doesn't contain proto_to_asm_rv32i.cpp?

This patch adds two new files: proto_to_asm_rv32i.cpp and rv32i_asm.proto. These files add to proto_to_asm.cpp and asm_proto.proto from the previous patch (https://reviews.llvm.org/D49521), which we chose to leave untouched so as to preserve the simpler example grammar that only contains the add and sub instructions.

Hi Jocelyn. What should I be referring to as the base of this patch? e.g. D49521 doesn't contain proto_to_asm_rv32i.cpp?