- User Since
- Aug 6 2013, 5:31 AM (268 w, 8 h)
Sun, Sep 23
Fri, Sep 21
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.
Thu, Sep 20
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.
Wed, Sep 19
Update to add a 'However' has suggested by @jyknight. Thanks for the review.
Tue, Sep 18
Closed by rL342487
Updated so common code for generating masks etc is reused through AtomicExpandPass.
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.
Thu, Sep 13
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.
Thu, Sep 6
Looks good to me, thanks. Just one tiny comment inline.
Looks good to me, thanks.
Thu, Aug 30
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!
Aug 24 2018
Really good catch - thanks for the fix!
Aug 23 2018
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.
Aug 17 2018
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?
Aug 16 2018
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.
Aug 15 2018
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.
Aug 8 2018
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.
Aug 3 2018
Aug 2 2018
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?