asb (Alex Bradbury)
Director and Co-founder, lowRISC CIC

Projects

User does not belong to any projects.

User Details

User Since
Aug 6 2013, 5:31 AM (268 w, 8 h)

Recent Activity

Sun, Sep 23

asb added a comment to D46423: [WIP, RISCV] Support .option relax and .option norelax.

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.

Sun, Sep 23, 3:42 AM

Fri, Sep 21

asb updated the diff for D52299: [RISCV][MC] Accept %lo and %pcrel_lo on operands to li.

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

Fri, Sep 21, 2:24 AM
asb updated the diff for D48131: [RISCV] Implement codegen for cmpxchg on RV32I.

Update to consistently use loophead/looptail labels in comments.

Fri, Sep 21, 2:07 AM

Thu, Sep 20

asb added a comment to D46423: [WIP, RISCV] Support .option relax and .option norelax.

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.

Thu, Sep 20, 7:34 AM
asb added a dependency for D52299: [RISCV][MC] Accept %lo and %pcrel_lo on operands to li: D52298: [RISCV][MC] Add support for evaluating constant symbols as immediates.
Thu, Sep 20, 6:11 AM
asb added a dependent revision for D52298: [RISCV][MC] Add support for evaluating constant symbols as immediates: D52299: [RISCV][MC] Accept %lo and %pcrel_lo on operands to li.
Thu, Sep 20, 6:11 AM
asb created D52299: [RISCV][MC] Accept %lo and %pcrel_lo on operands to li.
Thu, Sep 20, 6:11 AM
asb created D52298: [RISCV][MC] Add support for evaluating constant symbols as immediates.
Thu, Sep 20, 5:32 AM
asb committed rL342641: [RISCV][MC] Modify evaluateConstantImm interface to allow reuse from addExpr.
[RISCV][MC] Modify evaluateConstantImm interface to allow reuse from addExpr
Thu, Sep 20, 4:42 AM
asb committed rL342629: [RISCV][MC] Improve parsing of jal/j operands.
[RISCV][MC] Improve parsing of jal/j operands
Thu, Sep 20, 1:13 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Thu, Sep 20, 1:12 AM

Wed, Sep 19

asb added a dependent revision for D48131: [RISCV] Implement codegen for cmpxchg on RV32I: D52234: [docs][AtomicExpandPass] Document the alternate lowering strategy for part-word atomicrmw/cmpxchg.
Wed, Sep 19, 8:03 AM
asb added a dependency for D52234: [docs][AtomicExpandPass] Document the alternate lowering strategy for part-word atomicrmw/cmpxchg: D48131: [RISCV] Implement codegen for cmpxchg on RV32I.
Wed, Sep 19, 8:03 AM
asb updated the diff for D52234: [docs][AtomicExpandPass] Document the alternate lowering strategy for part-word atomicrmw/cmpxchg.

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

Wed, Sep 19, 8:03 AM
asb committed rL342550: [AtomicExpandPass]: Add a hook for custom cmpxchg expansion in IR.
[AtomicExpandPass]: Add a hook for custom cmpxchg expansion in IR
Wed, Sep 19, 7:55 AM
asb closed D48130: [AtomicExpandPass]: Add a hook for custom cmpxchg expansion in IR.
Wed, Sep 19, 7:54 AM
asb committed rL342534: [RISCV] Codegen for i8, i16, and i32 atomicrmw with RV32A.
[RISCV] Codegen for i8, i16, and i32 atomicrmw with RV32A
Wed, Sep 19, 3:58 AM
asb closed D47882: [RISCV] Codegen for i8, i16, and i32 atomicrmw with RV32A.
Wed, Sep 19, 3:58 AM

Tue, Sep 18

asb abandoned D47553: Add TargetLowering::shouldExpandAtomicToLibCall and query it from AtomicExpandPass.

Re-abandoning.

Tue, Sep 18, 8:21 AM
asb committed rL342488: [RISCV][MC] Use a custom ParserMethod for the bare_symbol operand type.
[RISCV][MC] Use a custom ParserMethod for the bare_symbol operand type
Tue, Sep 18, 8:19 AM
asb closed D51733: [RISCV][MC] Use a custom ParserMethod for the bare_symbol operand type.
Tue, Sep 18, 8:19 AM
asb added a comment to D51732: [RISCV][MC] Reject bare symbols for the simm12 operand type.

Closed by rL342487

Tue, Sep 18, 8:19 AM
asb abandoned D51732: [RISCV][MC] Reject bare symbols for the simm12 operand type.
Tue, Sep 18, 8:19 AM
asb committed rL342487: [RISCV][MC] Reject bare symbols for the simm12 operand type.
[RISCV][MC] Reject bare symbols for the simm12 operand type
Tue, Sep 18, 8:15 AM
asb committed rL342486: [RISCV][MC] Tighten up checking of sybol operands to lui and auipc.
[RISCV][MC] Tighten up checking of sybol operands to lui and auipc
Tue, Sep 18, 8:13 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Tue, Sep 18, 8:12 AM
asb updated subscribers of D52234: [docs][AtomicExpandPass] Document the alternate lowering strategy for part-word atomicrmw/cmpxchg.
Tue, Sep 18, 7:34 AM
asb created D52234: [docs][AtomicExpandPass] Document the alternate lowering strategy for part-word atomicrmw/cmpxchg.
Tue, Sep 18, 7:34 AM
asb added a comment to D48131: [RISCV] Implement codegen for cmpxchg on RV32I.

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

Tue, Sep 18, 7:28 AM
asb updated the diff for D48131: [RISCV] Implement codegen for cmpxchg on RV32I.

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

Tue, Sep 18, 7:23 AM
asb added a comment to D48130: [AtomicExpandPass]: Add a hook for custom cmpxchg expansion in IR.

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.

Tue, Sep 18, 7:17 AM
asb updated the diff for D48130: [AtomicExpandPass]: Add a hook for custom cmpxchg expansion in IR.

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.

Tue, Sep 18, 7:17 AM
asb updated the diff for D47882: [RISCV] Codegen for i8, i16, and i32 atomicrmw with RV32A.

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

Tue, Sep 18, 7:15 AM

Thu, Sep 13

asb accepted D52005: [RISCV][MC] Reject bare symbols for the simm6 and simm6nonzero operand types.

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

Thu, Sep 13, 7:27 AM
asb added a reviewer for D51828: [RISCV] Fix decoding of fence instruction with invalid field: asb.

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.

Thu, Sep 13, 7:18 AM
asb accepted D51815: [RISCV] Fix decoding of invalid instruction with C extension enabled..

Thanks, looks good to me.

Thu, Sep 13, 7:06 AM
asb created D52029: [RISCV][MC] Improve parsing of jal/j operands.
Thu, Sep 13, 5:53 AM

Thu, Sep 6

asb added inline comments to D46759: [RISCV] Support named operands for CSR instructions..
Thu, Sep 6, 8:13 AM
asb accepted D51705: [RISCV] Fix crash in decoding instruction with unknown floating point rounding mode.

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

Thu, Sep 6, 7:24 AM
asb accepted D51708: [RISCV] Fix AddressSanitizer heap-buffer-overflow in disassembling.

Looks good to me, thanks.

Thu, Sep 6, 7:18 AM
asb created D51733: [RISCV][MC] Use a custom ParserMethod for the bare_symbol operand type.
Thu, Sep 6, 6:54 AM
asb created D51732: [RISCV][MC] Reject bare symbols for the simm12 operand type.
Thu, Sep 6, 6:53 AM
asb created D51731: [RISCV][MC] Tighten up checking of sybol operands to lui and auipc.
Thu, Sep 6, 6:47 AM
asb committed rL341546: [RISCV][NFC] Rework test/MC/RISCV/rv{32,64}* to allow testing of symbol operands.
[RISCV][NFC] Rework test/MC/RISCV/rv{32,64}* to allow testing of symbol operands
Thu, Sep 6, 6:42 AM

Thu, Aug 30

asb added a comment to D51144: Implemented Protobuf fuzzer for LLVM MC Assembler.

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.

Thu, Aug 30, 7:28 AM
asb accepted D50246: [RISCV] Add support for computing sysroot for riscv32-unknown-elf.

Looks good to me, thanks!

Thu, Aug 30, 6:16 AM
asb accepted D50790: [RISCV] Fixed SmallVector.h Assertion `idx < size()'.
Thu, Aug 30, 6:15 AM
asb added a reviewer for D50790: [RISCV] Fixed SmallVector.h Assertion `idx < size()': asb.

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

Thu, Aug 30, 6:15 AM
asb committed rL341053: [RISCV] Fix r341050.
[RISCV] Fix r341050
Thu, Aug 30, 3:40 AM
asb committed rL341050: [RISCV][NFC] Rework CHECK lines in rvi-aliases-valid.s.
[RISCV][NFC] Rework CHECK lines in rvi-aliases-valid.s
Thu, Aug 30, 3:26 AM

Aug 24 2018

asb accepted D51217: [RISCV] atomic_store_nn have a different layout to regular store.

Really good catch - thanks for the fix!

Aug 24 2018, 10:43 AM

Aug 23 2018

asb added a comment to D47882: [RISCV] Codegen for i8, i16, and i32 atomicrmw with RV32A.

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.

Aug 23 2018, 9:06 AM
asb added a comment to D46423: [WIP, RISCV] Support .option relax and .option norelax.

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.

Aug 23 2018, 7:46 AM
asb added inline comments to D50808: [MC, RISCV] Fixed StringRef Assertion `Index < Length && "Invalid index!"'.
Aug 23 2018, 7:21 AM
asb added a comment to D51148: [RISCV] Fix std::advance slowness.

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

Aug 23 2018, 7:13 AM
asb updated the diff for D47882: [RISCV] Codegen for i8, i16, and i32 atomicrmw with RV32A.

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

Aug 23 2018, 7:02 AM
asb added a comment to D50246: [RISCV] Add support for computing sysroot for riscv32-unknown-elf.

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.

Aug 23 2018, 6:09 AM
asb added a comment to D46423: [WIP, RISCV] Support .option relax and .option norelax.

Many thanks for the update.

Aug 23 2018, 5:58 AM
asb accepted D50797: [RISCV] Fixed Assertion`Kind == Immediate && "Invalid type access!"' failed..

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

Aug 23 2018, 5:17 AM
asb added a reviewer for D50797: [RISCV] Fixed Assertion`Kind == Immediate && "Invalid type access!"' failed.: asb.
Aug 23 2018, 5:14 AM
asb accepted D51148: [RISCV] Fix std::advance slowness.

Thanks, looks good to me.

Aug 23 2018, 5:13 AM
asb added a reviewer for D51148: [RISCV] Fix std::advance slowness: asb.
Aug 23 2018, 5:06 AM
asb added a comment to D50790: [RISCV] Fixed SmallVector.h Assertion `idx < size()'.

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

Aug 23 2018, 5:03 AM
asb added a reviewer for D50790: [RISCV] Fixed SmallVector.h Assertion `idx < size()': asb.
Aug 23 2018, 4:45 AM
asb accepted D50043: [RISCV] RISC-V using -fuse-init-array by default.

Thanks, looks good to me.

Aug 23 2018, 4:42 AM

Aug 17 2018

asb committed rL340027: [AtomicExpandPass] Widen partword atomicrmw or/xor/and before tryExpandAtomicRMW.
[AtomicExpandPass] Widen partword atomicrmw or/xor/and before tryExpandAtomicRMW
Aug 17 2018, 7:04 AM
asb closed D48129: [AtomicExpandPass] Widen partword atomicrmw or/xor/and before tryExpandAtomicRMW.
Aug 17 2018, 7:04 AM
asb added a comment to D47755: [RISCV] Insert R_RISCV_ALIGN relocation type and Nops for alignment when linker relaxation enabled.

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 17 2018, 5:29 AM

Aug 16 2018

asb accepted D50836: [RISCV] Remove unused function.

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

Aug 16 2018, 6:45 AM
asb added a comment to D47755: [RISCV] Insert R_RISCV_ALIGN relocation type and Nops for alignment when linker relaxation enabled.

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.

Aug 16 2018, 6:40 AM
asb added a comment to D45961: [MC] Add MCSubtargetInfo to MCAlignFragment [NFC].

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

Aug 16 2018, 6:38 AM
asb requested changes to D50043: [RISCV] RISC-V using -fuse-init-array by default.
Aug 16 2018, 4:30 AM
asb committed rL339864: [RISCV][MC] Don't fold symbol differences if requiresDiffExpressionRelocations….
[RISCV][MC] Don't fold symbol differences if requiresDiffExpressionRelocations…
Aug 16 2018, 4:27 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Aug 16 2018, 4:27 AM
asb added a dependent revision for D48129: [AtomicExpandPass] Widen partword atomicrmw or/xor/and before tryExpandAtomicRMW: D47882: [RISCV] Codegen for i8, i16, and i32 atomicrmw with RV32A.
Aug 16 2018, 3:44 AM
asb added a dependency for D47882: [RISCV] Codegen for i8, i16, and i32 atomicrmw with RV32A: D48129: [AtomicExpandPass] Widen partword atomicrmw or/xor/and before tryExpandAtomicRMW.
Aug 16 2018, 3:44 AM
asb added inline comments to D47882: [RISCV] Codegen for i8, i16, and i32 atomicrmw with RV32A.
Aug 16 2018, 3:43 AM
asb updated the diff for D47882: [RISCV] Codegen for i8, i16, and i32 atomicrmw with RV32A.

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

Aug 16 2018, 3:43 AM
asb added a comment to D46759: [RISCV] Support named operands for CSR instructions..

Thanks Ana, some initial thoughts:

Aug 16 2018, 3:13 AM
asb removed a dependent revision for D48129: [AtomicExpandPass] Widen partword atomicrmw or/xor/and before tryExpandAtomicRMW: D48131: [RISCV] Implement codegen for cmpxchg on RV32I.
Aug 16 2018, 2:17 AM
asb removed a dependency for D48131: [RISCV] Implement codegen for cmpxchg on RV32I: D48129: [AtomicExpandPass] Widen partword atomicrmw or/xor/and before tryExpandAtomicRMW.
Aug 16 2018, 2:17 AM
asb removed a dependent revision for D47882: [RISCV] Codegen for i8, i16, and i32 atomicrmw with RV32A: D48129: [AtomicExpandPass] Widen partword atomicrmw or/xor/and before tryExpandAtomicRMW.
Aug 16 2018, 2:17 AM
asb removed a dependency for D48129: [AtomicExpandPass] Widen partword atomicrmw or/xor/and before tryExpandAtomicRMW: D47882: [RISCV] Codegen for i8, i16, and i32 atomicrmw with RV32A.
Aug 16 2018, 2:17 AM
asb added inline comments to D47882: [RISCV] Codegen for i8, i16, and i32 atomicrmw with RV32A.
Aug 16 2018, 2:17 AM
asb updated the diff for D48129: [AtomicExpandPass] Widen partword atomicrmw or/xor/and before tryExpandAtomicRMW.

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 16 2018, 2:15 AM

Aug 15 2018

asb requested changes to D46677: [RISCV] Add R_RISCV_RELAX relocation to all possible relax candidates..
Aug 15 2018, 8:09 AM
asb added a comment to D46424: [RISCV] Support .option push and .option pop.

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

Aug 15 2018, 7:45 AM
asb added a comment to D46423: [WIP, RISCV] Support .option relax and .option norelax.

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?

Aug 15 2018, 7:45 AM
asb added a comment to D45961: [MC] Add MCSubtargetInfo to MCAlignFragment [NFC].

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

Aug 15 2018, 6:51 AM
asb accepted D45960: [MC] Add MCSubtargetInfo to MCPaddingFragment [NFC].

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

Aug 15 2018, 6:37 AM

Aug 8 2018

asb updated the diff for D47882: [RISCV] Codegen for i8, i16, and i32 atomicrmw with RV32A.

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.

Aug 8 2018, 11:46 PM
asb committed rL339255: [RISCV] Add mnemonic alias: move, sbreak and scall..
[RISCV] Add mnemonic alias: move, sbreak and scall.
Aug 8 2018, 7:54 AM
asb closed D50217: [RISCV] Add mnemonic alias: move, sbreak and scall..
Aug 8 2018, 7:54 AM
asb committed rL339252: [RISCV] Add InstAlias definitions for add[w], and, xor, or, sll[w], srl[w], sra….
[RISCV] Add InstAlias definitions for add[w], and, xor, or, sll[w], srl[w], sra…
Aug 8 2018, 7:46 AM
asb closed D50046: [RISCV] Add InstAlias definitions for add[w], and, xor, or, sll[w], srl[w], sra[w], slt and sltu with immediate..
Aug 8 2018, 7:46 AM
asb accepted D50046: [RISCV] Add InstAlias definitions for add[w], and, xor, or, sll[w], srl[w], sra[w], slt and sltu with immediate..

Thanks Kito, looks good to me.

Aug 8 2018, 7:44 AM
asb accepted D50217: [RISCV] Add mnemonic alias: move, sbreak and scall..

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

Aug 8 2018, 7:39 AM
asb accepted D49661: [RISCV] Add "lla" pseudo-instruction to assembler.

Thanks, this looks good to me.

Aug 8 2018, 7:35 AM

Aug 3 2018

asb added a comment to D50164: Adding proto-fuzzer support for more extensions with new driver flow.
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.

Aug 3 2018, 4:14 AM

Aug 2 2018

asb added a comment to D50164: Adding proto-fuzzer support for more extensions with new driver flow.

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?

Aug 2 2018, 8:31 AM