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 (215 w, 5 d)

Recent Activity

Fri, Sep 22

asb added inline comments to D29934: [RISCV 12/n] Codegen support for memory operations.
Fri, Sep 22, 9:00 AM

Thu, Sep 21

asb added a comment to D37898: [TargetLowering] Correctly track NumFixedArgs field of CallLoweringInfo.

Ping? Any concerns from an ARM/AArch64/X86 perspective?

Thu, Sep 21, 11:59 PM

Mon, Sep 18

asb committed rL313534: Add myself to CREDITS.txt.
Add myself to CREDITS.txt
Mon, Sep 18, 7:35 AM

Sun, Sep 17

asb added a comment to D37957: [TableGen] Some simple optimizations to TableGen execution time.

In my view, disabling formatting is unhelpful when you have to debug a compilation error is generated from tablegen'ed code or when auditing its output. If the build-time improvement is meaningful, I'd suggest disabling formatting only when building LLVM in release mode.

Sun, Sep 17, 9:19 AM
asb updated the diff for D29934: [RISCV 12/n] Codegen support for memory operations.

Update based on review comments (thanks!). This version of the patch also adds (and tests) support for sextload/zextload/anyextload of an i1 value.

Sun, Sep 17, 8:53 AM
asb added inline comments to D29933: [RISCV 11/n] Initial codegen support for ALU operations.
Sun, Sep 17, 8:46 AM
asb updated the diff for D29933: [RISCV 11/n] Initial codegen support for ALU operations.

Update based on review comments (thanks!).

Sun, Sep 17, 8:45 AM
asb updated the diff for D23568: [RISCV 10/10] Add common fixups and relocations.

Refresh patch based on updates to others in series.

Sun, Sep 17, 7:52 AM
asb committed rL313486: [RISCV] Add support for disassembly.
[RISCV] Add support for disassembly
Sun, Sep 17, 7:38 AM
asb closed D23567: [RISCV 9/10] Add support for disassembly by committing rL313486: [RISCV] Add support for disassembly.
Sun, Sep 17, 7:38 AM
asb committed rL313485: [RISCV] Add support for all RV32I instructions.
[RISCV] Add support for all RV32I instructions
Sun, Sep 17, 7:29 AM
asb closed D23566: [RISCV 8/10] Add support for all RV32I instructions by committing rL313485: [RISCV] Add support for all RV32I instructions.
Sun, Sep 17, 7:29 AM
asb added inline comments to D23566: [RISCV 8/10] Add support for all RV32I instructions.
Sun, Sep 17, 7:28 AM

Fri, Sep 15

asb created D37898: [TargetLowering] Correctly track NumFixedArgs field of CallLoweringInfo.
Fri, Sep 15, 2:30 AM

Wed, Sep 13

asb updated the diff for D37798: Teach TargetInstrInfo::getInlineAsmLength to parse .space directives with integer arguments.

Sorry for the noise, the previous version left out part of the patch.

Wed, Sep 13, 9:44 AM
asb updated the diff for D37798: Teach TargetInstrInfo::getInlineAsmLength to parse .space directives with integer arguments.

Update to address review comments from @sdardis (thanks, good catch).

Wed, Sep 13, 9:42 AM
asb created D37798: Teach TargetInstrInfo::getInlineAsmLength to parse .space directives with integer arguments.
Wed, Sep 13, 3:28 AM

Thu, Sep 7

asb accepted D31951: TableGen support for parameterized register class information.

I'm adding my accept to this. I carefully reviewed the details of the code once it was first posted (see earlier comments), and believe this is a valuable improvement. There are others on the review list with more TableGen who may be better placed to comment on the high level design decisions. If somebody else with more TableGen insight (e.g. @sdardis, @MatzeB) can add their LGTM I think this is ready to go on.

Thu, Sep 7, 1:10 PM
asb added inline comments to D29933: [RISCV 11/n] Initial codegen support for ALU operations.
Thu, Sep 7, 7:09 AM
asb committed rL312713: [Sparc][NFC] Clean up SelectCC lowering.
[Sparc][NFC] Clean up SelectCC lowering
Thu, Sep 7, 4:32 AM
asb closed D37194: [Sparc][NFC] Clean up SelectCC lowering by committing rL312713: [Sparc][NFC] Clean up SelectCC lowering.
Thu, Sep 7, 4:32 AM
asb updated the diff for D29937: [RISCV 15/n] Implement lowering of ISD::SELECT_CC.

Full stops at end of comments.

Thu, Sep 7, 4:05 AM
asb updated the diff for D29934: [RISCV 12/n] Codegen support for memory operations.

Fix indentation (thanks @mgrang!).

Thu, Sep 7, 4:02 AM
asb added inline comments to D29934: [RISCV 12/n] Codegen support for memory operations.
Thu, Sep 7, 4:02 AM
asb added inline comments to D29933: [RISCV 11/n] Initial codegen support for ALU operations.
Thu, Sep 7, 3:36 AM
asb updated the diff for D29933: [RISCV 11/n] Initial codegen support for ALU operations.

Address review comments from @mgrang (thanks!).

Thu, Sep 7, 3:32 AM
asb updated the diff for D23568: [RISCV 10/10] Add common fixups and relocations.

Update to address review comments (thanks @mgrang!).

Thu, Sep 7, 3:22 AM
asb added inline comments to D23566: [RISCV 8/10] Add support for all RV32I instructions.
Thu, Sep 7, 3:12 AM

Wed, Sep 6

asb added a comment to D23566: [RISCV 8/10] Add support for all RV32I instructions.

I think a number of future RISC-V backend patches will be straight-forward enough to just use post-commit review. However, the developer policy specifically warns against abandoning the review process and committing directly once a patch has been submitted https://llvm.org/docs/DeveloperPolicy.html#code-reviews

Wed, Sep 6, 7:41 AM
asb committed rL312624: [RISCV][NFC] Fix sorting of includes in lib/Target/RISCV.
[RISCV][NFC] Fix sorting of includes in lib/Target/RISCV
Wed, Sep 6, 2:22 AM

Tue, Sep 5

asb added a comment to D23568: [RISCV 10/10] Add common fixups and relocations.

Ping?

Tue, Sep 5, 12:19 PM
asb added a comment to D23566: [RISCV 8/10] Add support for all RV32I instructions.

Ping?

Tue, Sep 5, 12:19 PM

Thu, Aug 31

asb committed rL312236: [Docs] Update CodingStandards to recommend range-based for loops.
[Docs] Update CodingStandards to recommend range-based for loops
Thu, Aug 31, 5:35 AM
asb closed D37264: [Docs] Update CodingStandards to recommend range-based for loops by committing rL312236: [Docs] Update CodingStandards to recommend range-based for loops.
Thu, Aug 31, 5:35 AM

Wed, Aug 30

asb updated the diff for D37264: [Docs] Update CodingStandards to recommend range-based for loops.

Thank you everybody for the feedback. I've made several updates vs the previous version of the patch:

  • Don't use auto for the element type in range-based for loops
  • Retain most of the previous text on evaluating end() every time for a loop. I initially thought I could relegate this to a shorter note given that explicit iterator-based loops should be relatively rare, but my attempt to do this lost important points.
  • Do use auto for the iterator (hopefully this is uncontroversial, as it is explicitly suggested in the "Use `auto` Type Deduction to Make Code More Readable" section
Wed, Aug 30, 5:49 AM

Tue, Aug 29

asb updated the diff for D37264: [Docs] Update CodingStandards to recommend range-based for loops.

As requested by @majnemer, this updated version of the patch adds in some more explanation about re-evaluating end() and mutation.

Tue, Aug 29, 10:49 AM
asb added inline comments to D37264: [Docs] Update CodingStandards to recommend range-based for loops.
Tue, Aug 29, 10:35 AM
asb added inline comments to D37264: [Docs] Update CodingStandards to recommend range-based for loops.
Tue, Aug 29, 10:33 AM
asb added inline comments to D37264: [Docs] Update CodingStandards to recommend range-based for loops.
Tue, Aug 29, 10:25 AM
asb created D37264: [Docs] Update CodingStandards to recommend range-based for loops.
Tue, Aug 29, 8:18 AM

Mon, Aug 28

asb created D37230: Set hasSideEffects=0 for TargetOpcode::BUNDLE.
Mon, Aug 28, 1:28 PM
asb accepted D37199: [ARM] - Tidy-up ARMAsmPrinter.cpp.

This is a straight-forward cleanup which looks good to me.

Mon, Aug 28, 8:52 AM

Sun, Aug 27

asb created D37194: [Sparc][NFC] Clean up SelectCC lowering.
Sun, Aug 27, 7:26 AM

Sat, Aug 26

asb updated subscribers of D37052: Add default address space for functions to the data layout (1/4).
Sat, Aug 26, 3:54 PM

Fri, Aug 25

asb updated the diff for D29937: [RISCV 15/n] Implement lowering of ISD::SELECT_CC.

Update to put the 'default:' case first in a switch (thanks @apazos).

Fri, Aug 25, 2:38 PM
asb added inline comments to D29935: [RISCV 13/n] Codegen for conditional branches.
Fri, Aug 25, 2:33 PM
asb updated the diff for D29936: [RISCV 14/n] Support for function calls.

Fix line that was over 80 columns (thanks @apazos).

Fri, Aug 25, 2:30 PM
asb updated the diff for D23566: [RISCV 8/10] Add support for all RV32I instructions.

Address comments from @apazos (thanks!). I've also converted a few if conditions to use MCAsmLexer::{is,isNot}.

Fri, Aug 25, 2:25 PM
asb added a comment to D37118: [ARM] Tidy-up ARMAsmParser. NFC..

Looks good to me. I agree it seems almost unnecessary to have a helper for MRI->getSubReg(QReg, ARM::dsub_0), but as Javed points out it's called many times and getDRegFromQReg is much more readable.

Fri, Aug 25, 1:13 PM

Aug 25 2017

asb added a comment to D37118: [ARM] Tidy-up ARMAsmParser. NFC..

Why not use MCRegisterInfo::getSubReg for this purpose, thus avoiding repeating information that's already present in ARMRegisterInfo.td? MRI->getSubReg(QReg, ARM::dsub_0) should replace the functionality of getDRegFromQReg.

Aug 25 2017, 7:00 AM
asb added inline comments to D29937: [RISCV 15/n] Implement lowering of ISD::SELECT_CC.
Aug 25 2017, 6:20 AM
asb updated the diff for D29937: [RISCV 15/n] Implement lowering of ISD::SELECT_CC.

Hi Philip, many many thanks for the review - this code has been copied around several backends, and we can certainly do better (patches for those backends to follow!). This is exactly the sort of thing I'm trying to avoid with this backend project, so I really appreciate you calling it this case where the code fell short. The code comment was incorrect to say a diamond control flow pattern is being inserted, as it's actually just a triangle. Hopefully the Head/IfFalse/Tail names make more sense. I've made normaliseSetCC a separate helper function in addition to your suggested CondCode->BranchOpcode helper (this felt sensible, given getBranchOpcodeForIntCondCode expects its argument has been normalised in this way).

Aug 25 2017, 6:20 AM
asb updated subscribers of D37065: Ensure standard pseudo instructions (TargetOpcode::*) are compatible with guessInstructionProperties=0.
Aug 25 2017, 5:40 AM

Aug 24 2017

asb updated the diff for D37097: Set hasSideEffects=0 for PHI and fix passes relying isSafeToMove/hasUnmodeledSideEffects being true for PHI.

Update to have MachineInstr::isSafeToMove return false for PHI, as requested by @qcolombet.

Aug 24 2017, 11:09 AM
asb added inline comments to D37052: Add default address space for functions to the data layout (1/4).
Aug 24 2017, 10:34 AM
asb accepted D37051: Model cache size and associativity in TargetTransformInfo.

Looks good to me.

Aug 24 2017, 1:51 AM
asb added a comment to D23566: [RISCV 8/10] Add support for all RV32I instructions.

Ping?

Aug 24 2017, 1:33 AM
asb added a comment to D37065: Ensure standard pseudo instructions (TargetOpcode::*) are compatible with guessInstructionProperties=0.

Hi Quentin, D37097 sets hasSideEffects=0 for PHI and fixes the passes that relied on the old value. I've separated out that change, as I think it makes most sense to commit this patch which just makes the currently inferred flags explicit, then to follow up and fix the flags that were questionable.

Aug 24 2017, 1:26 AM
asb updated the diff for D37097: Set hasSideEffects=0 for PHI and fix passes relying isSafeToMove/hasUnmodeledSideEffects being true for PHI.

Attach the correct patch.

Aug 24 2017, 1:24 AM
asb added a dependency for D37097: Set hasSideEffects=0 for PHI and fix passes relying isSafeToMove/hasUnmodeledSideEffects being true for PHI: D37065: Ensure standard pseudo instructions (TargetOpcode::*) are compatible with guessInstructionProperties=0.
Aug 24 2017, 1:21 AM
asb added a dependent revision for D37065: Ensure standard pseudo instructions (TargetOpcode::*) are compatible with guessInstructionProperties=0: D37097: Set hasSideEffects=0 for PHI and fix passes relying isSafeToMove/hasUnmodeledSideEffects being true for PHI.
Aug 24 2017, 1:21 AM
asb created D37097: Set hasSideEffects=0 for PHI and fix passes relying isSafeToMove/hasUnmodeledSideEffects being true for PHI.
Aug 24 2017, 1:20 AM

Aug 23 2017

asb added inline comments to D37051: Model cache size and associativity in TargetTransformInfo.
Aug 23 2017, 11:40 PM
asb added a comment to D37051: Model cache size and associativity in TargetTransformInfo.

Agreed, Returning the number of cache lines as result of getCacheSize wouldn't be a good idea.

I meant using the cache line size in X86TTIImpl::getCacheSize. This was just a minor potential nit, I think either way is fine.

Aug 23 2017, 2:48 PM
asb updated the diff for D29938: [RISCV 16/n] Support and tests for a variety of additional LLVM IR constructs.

Update to reflect split of instruction and pattern definitions, as discussed in http://lists.llvm.org/pipermail/llvm-dev/2017-August/116635.html

Aug 23 2017, 2:28 PM
asb updated the diff for D29937: [RISCV 15/n] Implement lowering of ISD::SELECT_CC.

Instruction definition and patterns have been split, as discussed in http://lists.llvm.org/pipermail/llvm-dev/2017-August/116635.html.

Aug 23 2017, 2:27 PM
asb updated the diff for D29936: [RISCV 14/n] Support for function calls.

Instruction definitions and patterns have been split, as discussed in http://lists.llvm.org/pipermail/llvm-dev/2017-August/116635.html

Aug 23 2017, 2:26 PM
asb updated the diff for D29935: [RISCV 13/n] Codegen for conditional branches.

Instruction definitions and patterns have been split, as discussed in http://lists.llvm.org/pipermail/llvm-dev/2017-August/116635.html. There is no functional change vs the accepted patch revision.

Aug 23 2017, 2:24 PM
asb updated the diff for D37065: Ensure standard pseudo instructions (TargetOpcode::*) are compatible with guessInstructionProperties=0.

The explanatory comment had the wrong polarity for guessInstructionProperties. This update fixes that.

Aug 23 2017, 2:23 PM
asb updated the diff for D29934: [RISCV 12/n] Codegen support for memory operations.

Instructions definitions and patterns have been split, as discussed in http://lists.llvm.org/pipermail/llvm-dev/2017-August/116635.html

Aug 23 2017, 2:23 PM
asb updated the diff for D29933: [RISCV 11/n] Initial codegen support for ALU operations.

This patch has been updated to split out instruction definitions from patterns, as discussed here and on llvm-dev. The RISCV backend now compiles with guessInstructionProperties=0. It is now dependent on D37065, which fixes the target independent TargetOpcode::* instructions to work when this option is enabled.

Aug 23 2017, 2:18 PM
asb added a comment to D37051: Model cache size and associativity in TargetTransformInfo.

Size in bytes makes most sense to me. The first thing any caller is going to do is convert away from cache lines to bytes or kilobytes anyway.

Aug 23 2017, 1:56 PM
asb added inline comments to D37065: Ensure standard pseudo instructions (TargetOpcode::*) are compatible with guessInstructionProperties=0.
Aug 23 2017, 11:10 AM
asb added inline comments to D37065: Ensure standard pseudo instructions (TargetOpcode::*) are compatible with guessInstructionProperties=0.
Aug 23 2017, 10:14 AM
asb updated the diff for D37065: Ensure standard pseudo instructions (TargetOpcode::*) are compatible with guessInstructionProperties=0.

The initial version of this patch wrongly focused on GenericOpcodes.td. Updated to tackle the problem head-on in Target.td. I've removed the RFC label as the updated version of the patch seems less likely to be controversial.

Aug 23 2017, 8:52 AM
asb added a comment to D37065: Ensure standard pseudo instructions (TargetOpcode::*) are compatible with guessInstructionProperties=0.

It seems there are a number of other instructions in Target.td that also have this problem. guessInstructionProperties can't be enabled for a target without patching target-independent code, and this patch was incorrect to suggest GlobalISel was the only culprit. Let me look at this s ome more...

Aug 23 2017, 8:29 AM
asb retitled D37065: Ensure standard pseudo instructions (TargetOpcode::*) are compatible with guessInstructionProperties=0 from [RFC] Stop GenericOpcodes.td from breaking targets that set guessInstructionProperties=0 to [RFC][GlobalISel] Stop GenericOpcodes.td from breaking targets that set guessInstructionProperties=0.
Aug 23 2017, 8:09 AM
asb created D37065: Ensure standard pseudo instructions (TargetOpcode::*) are compatible with guessInstructionProperties=0.
Aug 23 2017, 8:09 AM
asb added a comment to D37052: Add default address space for functions to the data layout (1/4).

From the perspective of "does the code do what it intends to and meet LLVM coding standards?" this looks good to me.

Aug 23 2017, 7:07 AM
asb updated the diff for D23568: [RISCV 10/10] Add common fixups and relocations.

Just spotted a minor issue. RISCVMCExpr::getVariantKindForName would be better returning VK_RISCV_Invalid if the name is unrecognised rather htna VK_RISCV_None. Updated diff fixes this.

Aug 23 2017, 6:48 AM
asb updated the diff for D23568: [RISCV 10/10] Add common fixups and relocations.

Previous versions of this patch were too accepting of invalid inputs, e.g. accepting addi a0, a0, %hi(1). Previously there was logic that would try to evaluate a RISCVMCExpr as a constant in createImm (i.e. as early as possible). This throws away information about any operand modifier that was parsed. Therefore, move to evaluating the constant in in the isFoo() predicate arguments, which also checks whether any operand modifiers were allowed. rv32i-invalid.s has been expanded to systematically test these issues. As suggested by Dylan, I've also added a helper function to factor out the commonality between the isSImmNNLsb0 functions.

Aug 23 2017, 6:44 AM
asb added a comment to D33574: PPC: Verify that branch fixups fit within the range..

I think that the test here is: is this something that a user could hit, (including with inline assembly, etc.), or does this indicate an internal problem only? For internal errors, we have lots of checks that we optimize out for release builds (and there's always the option of building release+asserts, and I deploy those builds on more experimental platforms as a general rule). If it is something that a user could hit, then it should be an error that the user will always get.

Aug 23 2017, 5:54 AM
asb added a comment to D23568: [RISCV 10/10] Add common fixups and relocations.

Thanks for the comments Dylan. Playing around with this, I've noticed that the MC assembler is too accepting of some inputs (e.g. accepting addi a1, a2, %hi(foo) where gas would reject it). I'm fixing this and expanding tests now, which will change many of the isFoo functions anyway. I'll watch for the else after return thing. I personally don't find that guideline very compelling for else if cases where removing the else fails to reduce indentation, but I shall of course conform with LLVM norms :)

Aug 23 2017, 4:20 AM
asb added inline comments to D37051: Model cache size and associativity in TargetTransformInfo.
Aug 23 2017, 3:55 AM
asb added inline comments to D37051: Model cache size and associativity in TargetTransformInfo.
Aug 23 2017, 1:07 AM
asb committed rL311531: [Lanai] Remove dead functions from LanaiRegisterInfo.
[Lanai] Remove dead functions from LanaiRegisterInfo
Aug 23 2017, 12:17 AM
asb closed D36829: [Lanai] Remove dead functions from LanaiRegisterInfo by committing rL311531: [Lanai] Remove dead functions from LanaiRegisterInfo.
Aug 23 2017, 12:17 AM

Aug 22 2017

asb added a comment to D33574: PPC: Verify that branch fixups fit within the range..

Hi folks, I think llvm_unreachable is really the wrong error handling strategy here. It _won't_ necessarily cause the process to exit on a release build. This is actually one of the cases where it is possible to report a nice error without too much hassle. In rL299529 I added an MCContext parameter to MCAsmBackend::applyFixup in order to make it easy to use MCContext::reportError for effort reporting in helper functions like adjustFixupValue. Due to later upstream changes, you now get hold of your MCContext via the MCAssembler argument. You can see an example of this approach in action in D23568.

Aug 22 2017, 11:09 PM
asb added a comment to D37020: [AArch64][Falkor] Avoid generating STRQro* instructions.

One of the usual AArch64 suspects should confirm, but this looks good to me.

Aug 22 2017, 1:31 PM
asb added a comment to D36964: [AArch64] ISel legalization debug messages. NFCI..

Hi Alex, thanks for the feedback. The difficulty here is finding the right balance of adding (good) debug messages, and not cluttering the code too much. I think what you are suggesting is what I had in my first revision: that had debug messages in the different AArch64 lowering functions. Then Sam's suggestion was to move some of these messages to SelectionDAG (which we did in D36984), which has 2 advantages: all targets benefit, and it does not clutter up the code. Also, I think this is quite difficult to get completely right the first time. So I think we will have to see what works and what is good and I will be adding more stuff as I go along. There's a lot of room for improvement here, but I already find these extra debug messages quite useful...

Aug 22 2017, 9:56 AM
asb updated the diff for D29937: [RISCV 15/n] Implement lowering of ISD::SELECT_CC.

Update to fix line > 80 chars.

Aug 22 2017, 8:18 AM
asb updated the diff for D29935: [RISCV 13/n] Codegen for conditional branches.

Update to address review comments. Thanks again @dylanmckay. If you're happy with this, please mark it accepted in Phabricator (makes it a little easier for seeing the current status of the RISC-V patchset at a glance).

Aug 22 2017, 7:59 AM
asb added a comment to D36964: [AArch64] ISel legalization debug messages. NFCI..

I think this is a really great initiative. I've added one inline comment. In addition to that, I have a general note. My natural instinct would have been to try to print a more informative message in the caller of the various is* functions (such as isLegalArithImmed). e.g. might better debug output be produced if debug printing were added to getAArch64Cmp instead?

Aug 22 2017, 7:54 AM
asb added inline comments to D23568: [RISCV 10/10] Add common fixups and relocations.
Aug 22 2017, 6:15 AM
asb updated the diff for D23568: [RISCV 10/10] Add common fixups and relocations.

Address review comments from @dylanmckay (thanks!).

Aug 22 2017, 6:15 AM
asb committed rL311435: Use report_fatal_error for unsupported calling conventions.
Use report_fatal_error for unsupported calling conventions
Aug 22 2017, 2:12 AM
asb closed D36830: Use report_fatal_error for unsupported calling conventions by committing rL311435: Use report_fatal_error for unsupported calling conventions.
Aug 22 2017, 2:12 AM

Aug 21 2017

asb added a comment to D29935: [RISCV 13/n] Codegen for conditional branches.

Are there any tests for the stack slot loading/storing? If there are some in a later patch, that's fine too.

Aug 21 2017, 8:07 AM
asb updated the diff for D29935: [RISCV 13/n] Codegen for conditional branches.

Updated to address review comments from @dylanmckay (thanks!).

Aug 21 2017, 6:51 AM

Aug 20 2017

asb added a comment to D36331: Add ARC backend.

Are experimental targets included in All targets? If not, do we have build bot which build experimental targets? Just to make sure that API of headers changes will not break them.

Aug 20 2017, 11:52 PM