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 (246 w, 1 h)

Recent Activity

Today

asb added a comment to D45859: [RISCV] Support "call" pseudoinstruction in the MC layer.

Thanks for the updates Shiva. I've added a few more suggested changes, and I think we're almost there.

Tue, Apr 24, 6:36 AM

Yesterday

asb accepted D44885: [RISCV] Expand function call to "call" pseudoinstruction.

Thanks Shiva. I think just two outstanding issues. Some of the test files in test/CodeGen/RISCV that weren't generated with update_llc_test_checks.py have unwanted changes in this patch. With that fixed, this is looking good to me.

Mon, Apr 23, 9:13 AM
asb added a comment to D45284: [RISCV] More validations on the input value of -march=.

Hi Alex, it seems the table expects these extensions in a canonical order too: all x extensions, followed by all s extensions, and then all sx extensions.

I can make the change, no problem. I have also coded other error situations described below.

But f I cannot push a test we can enable, because we error out when we find the first non-supported extension in the string with unsupported extension message, and stop parsing the string.

Mon, Apr 23, 8:54 AM
asb requested changes to D45859: [RISCV] Support "call" pseudoinstruction in the MC layer.
Mon, Apr 23, 7:49 AM

Thu, Apr 19

asb added a comment to D39053: [Bitfield] Add more cases to making the bitfield a separate location.

Hi @spetrovic - I think Hal Finkel's earlier request to update the comments of IsBetterAsSingleFieldRun remains unaddressed. It looks like at least the comment string immediately before auto IsBetterAsSingleFieldRun needs to be updated to reflect the changed behaviour.

Thu, Apr 19, 6:36 AM
asb added a comment to D45284: [RISCV] More validations on the input value of -march=.

This is looking great, the only remaining code comment I have is that getExtensionFeatures needs a comment describing it.

Thu, Apr 19, 5:50 AM
asb requested changes to D44885: [RISCV] Expand function call to "call" pseudoinstruction.

Hi Shiva. Thanks for this, I think the best way of landing this is to split the MC and codegen changes. Could you please:

  • Make a new review that contains only the changes required to support call foo in the MC layer
  • Rebase this patch on top of that, and update the title to something like 'Expand function call to "call" pseudoinstruction'
Thu, Apr 19, 5:29 AM
asb added a comment to D45748: [RISCV] Separate base from offset in lowerGlobalAddress.

Thanks Sameer. This is definitely a win for a range of inputs, but I'm also seeing regressions. An extra instruction is now required for any standalone global access + offset. There are even some cases where the global access is going from 2 instructions to 4 instructions (global with very large offset, requiring lui+addi). Do you have any thoughts on addressing these cases?

Thu, Apr 19, 5:07 AM

Wed, Apr 18

asb added a comment to D41949: [RISCV] implement li pseudo instruction.

In the end I decided to temporarily revert this patch. I've committed patches that fill in the holes identified in testing, and added the straight-forward patch for imm32 with lo12=0.

  • Better tests for imm32_hi20_only (rL330274)
  • Improved testing of the codegen -> compression codepath (rL330288)
  • Test for constant subexpression elimination when materialising immediates (rL330291)
  • Add pattern for immediates with zero for the lower 12 bits (rL330293)
Wed, Apr 18, 2:01 PM
asb committed rL330294: [RISCV] Add test changes missed from rL330293.
[RISCV] Add test changes missed from rL330293
Wed, Apr 18, 1:39 PM
asb committed rL330293: [RISCV] Introduce pattern for materialising immediates with 0 for lower 12 bits.
[RISCV] Introduce pattern for materialising immediates with 0 for lower 12 bits
Wed, Apr 18, 1:37 PM
asb committed rL330291: [RISCV] Add imm-cse.ll test case.
[RISCV] Add imm-cse.ll test case
Wed, Apr 18, 1:28 PM
asb committed rL330288: [RISCV] Expand codegen -> compression sanity checks and move to a single file.
[RISCV] Expand codegen -> compression sanity checks and move to a single file
Wed, Apr 18, 1:20 PM
asb committed rL330281: Revert "[RISCV] implement li pseudo instruction".
Revert "[RISCV] implement li pseudo instruction"
Wed, Apr 18, 12:08 PM
asb added a reverting commit for rL330224: [RISCV] implement li pseudo instruction: rL330281: Revert "[RISCV] implement li pseudo instruction".
Wed, Apr 18, 12:08 PM
asb committed rL330274: [RISCV] Add specific tests for materialising imm32hi20 constants.
[RISCV] Add specific tests for materialising imm32hi20 constants
Wed, Apr 18, 9:46 AM
asb added a comment to D41949: [RISCV] implement li pseudo instruction.

Ok, I've had a good think about this issue. I was slightly over-eager in committing this last night. Something like PseudoLI seems necessary for the more complex materialisation logic required for 64-bit immediates in RV64, but we can do without for RV32. I've weighed up whether to revert and revise, or to make changes post commit

Wed, Apr 18, 9:22 AM
asb added a comment to D41949: [RISCV] implement li pseudo instruction.

I'm just working through this now. I'll play around with the options and update this thread within the next few hours.

Wed, Apr 18, 2:52 AM

Tue, Apr 17

asb committed rL330224: [RISCV] implement li pseudo instruction.
[RISCV] implement li pseudo instruction
Tue, Apr 17, 3:00 PM
asb closed D41949: [RISCV] implement li pseudo instruction.
Tue, Apr 17, 3:00 PM
asb added a comment to D41949: [RISCV] implement li pseudo instruction.

Thanks again Mario. I've reviewed the new RISCVISelDAGToDAG changes and just have a minor comment. This is also looking good when testing with the torture suite. I'll commit this as soon as you can confirm my minor query.

Tue, Apr 17, 6:42 AM
asb updated subscribers of D45181: [RISCV] Add diff relocation support for RISC-V.

CC @sdardis - micromips seems to use aggressive linker relaxation in binutils ld meaning this may have some current or future relevance to the Mips LLVM backend.

Tue, Apr 17, 5:59 AM
asb added a comment to D44887: [RISCV] Add shouldForceRelocationWithApplyFixup MC AsmBackend Target Hook.
In D44887#1069614, @asb wrote:

I think it should be if (!Resolved && WasForced) return fixupNeedsRelaxation(Fixup, Value, DF, Layout);

I don't think we want a call from fixupNeedsRelaxationAdvanced -> fixupNeedsRelaxation.

I thought we will need call fixupNeedsRelaxation, because WasForced is true means the symbol could be resovled, but the offset could fit in or not would need fixupNeedsRelaxation to identify.
Is that reasonable?

Tue, Apr 17, 2:10 AM
asb added a comment to D44887: [RISCV] Add shouldForceRelocationWithApplyFixup MC AsmBackend Target Hook.
In D44887#1068861, @asb wrote:

Could you please expand rv32-relaxation.s and rv64-relaxation.s so the test includes the behaviour for branches to an unresolved symbol?

I'm a bit concerned about adding shouldForceRelocationWithApplyFixup. It adds complexity by adding another way of achieving something very similar to the existing shouldForceRelocation, and its use means that the return value of evaluateFixup no longer matches the description in its docstring (the value isn't 'fixed' by the definition in the docstring, but evaluateFixup still returns true).

Hi Alex.
In fact, the value is fixed as docstring description. You could observe the test case rv32-relaxation.s. When the relaxation enables, the branch operands will fill with fixup value but also preserve the relocation types and that's the target hook shouldForceRelocationWithApplyFixup try to achieve.

Tue, Apr 17, 1:19 AM

Mon, Apr 16

asb added a comment to D44887: [RISCV] Add shouldForceRelocationWithApplyFixup MC AsmBackend Target Hook.

Could you please expand rv32-relaxation.s and rv64-relaxation.s so the test includes the behaviour for branches to an unresolved symbol?

Mon, Apr 16, 9:22 AM
asb requested changes to D44888: [RISCV] Default enable linker relaxation and add -mrelax, -mno-relax flags.

Thanks Kito. -mrelax and -mno-relax currently only affect the backend. For completeness, I think this patch needs to pass the appropriate flag to the linker depending on relax/no-relax.

Mon, Apr 16, 8:15 AM
asb added inline comments to D45181: [RISCV] Add diff relocation support for RISC-V.
Mon, Apr 16, 8:10 AM
asb accepted D45660: [RISCV] Fix assert message operator.

Thanks, that's definitely an error. Please do feel totally free to commit straight-forward trivial fixes like this without the need for pre-commit review.

Mon, Apr 16, 6:44 AM
asb added inline comments to D45284: [RISCV] More validations on the input value of -march=.
Mon, Apr 16, 4:25 AM

Fri, Apr 13

asb added a comment to D41949: [RISCV] implement li pseudo instruction.

I'd do the compressed changes in a different patch. Thanks for updating the peephole RISCVISelDAGToDAG, I'll review that bit ASAP and then commit. At a first look, it seems to handle this exactly as I would expect.

Fri, Apr 13, 9:47 AM

Thu, Apr 12

asb accepted D45583: [RISCV] Add c.mv rs1, rs2 pattern for addi rs1, rs2, 0.

Thanks! Looks good to me.

Thu, Apr 12, 12:13 PM
asb added inline comments to D45395: [RISCV] Implement tail call optimization.
Thu, Apr 12, 6:34 AM
asb added a comment to D45181: [RISCV] Add diff relocation support for RISC-V.

I think this patch and its tests would be best if based on D44886, so it can be clear which behaviour is conditional on linker relaxation being enabled.

Thu, Apr 12, 6:27 AM
asb added a comment to D44886: [RISCV] Support linker relax function call from auipc and jalr to jal.

Given that Simon's D45181 will be dependent on this (as it should really query whether linker relaxation is enabled), it probably makes sense to get this merged sooner rather than later.

Thu, Apr 12, 6:22 AM
asb added a comment to D44888: [RISCV] Default enable linker relaxation and add -mrelax, -mno-relax flags.

Looks good to me, just missing help text on the command line options.

Thu, Apr 12, 6:07 AM
asb added a comment to D44886: [RISCV] Support linker relax function call from auipc and jalr to jal.

This seems like a sensible intermediate step, even if it's not a 100% match for gcc behaviour.

Thu, Apr 12, 6:06 AM
asb accepted D44885: [RISCV] Expand function call to "call" pseudoinstruction.

Thanks Shiva. This looks good to me - just two additional minor suggestions prior to commit (add a TODO comment and switch to using dyn_cast in one place).

Thu, Apr 12, 5:55 AM
asb added a comment to D45284: [RISCV] More validations on the input value of -march=.

I've added a few comments on tweaking the error messages based on your tests.

Thu, Apr 12, 5:46 AM
asb accepted D45237: [RISCV] Fix logic to check if frame pointer should be used.

Thanks, looks good to me.

Thu, Apr 12, 5:36 AM
asb accepted D45560: [RISCV] Change function alignment to 4 bytes, and 2 bytes for RVC.

Thanks Kito, looks good to me.

Thu, Apr 12, 2:32 AM

Wed, Apr 11

asb committed rL329877: [RISCV] Codegen support for RV32D floating point comparison operations.
[RISCV] Codegen support for RV32D floating point comparison operations
Wed, Apr 11, 10:53 PM
asb committed rL329876: [RISCV] Codegen support for RV32D floating point conversion operations.
[RISCV] Codegen support for RV32D floating point conversion operations
Wed, Apr 11, 10:50 PM
asb committed rL329874: [RISCV] Add codegen support for RV32D floating point arithmetic operations.
[RISCV] Add codegen support for RV32D floating point arithmetic operations
Wed, Apr 11, 10:45 PM
asb committed rL329872: [RISCV] Add tests missed in r329871.
[RISCV] Add tests missed in r329871
Wed, Apr 11, 10:40 PM
asb committed rL329871: [RISCV] Codegen support for RV32D floating point load/store, fadd.d, calling….
[RISCV] Codegen support for RV32D floating point load/store, fadd.d, calling…
Wed, Apr 11, 10:40 PM

Fri, Apr 6

asb accepted D42780: [RISCV] CompressPat Tablegen-driven Instruction Compression.

Thanks, this looks good to me now.

Fri, Apr 6, 2:57 AM
asb accepted D45119: [RISCV] Override EmitToStreamer in RISCVAsmPrinter to handle missed compression opportunities.

Looks good to me. Thanks.

Fri, Apr 6, 2:56 AM

Thu, Apr 5

asb added a comment to D45181: [RISCV] Add diff relocation support for RISC-V.

From a quick look in binutils, it seems micromips could be tripped up by similar issues.

Thu, Apr 5, 4:12 AM
asb added a comment to D45181: [RISCV] Add diff relocation support for RISC-V.

Thanks for this Simon.

Thu, Apr 5, 4:08 AM
asb added inline comments to D44971: [RISCV] Override fixupNeedsRelaxationAdvanced to avoid MC relaxation always promote to 32-bit form when -mrelax enabled.
Thu, Apr 5, 3:51 AM
asb added a comment to D44886: [RISCV] Support linker relax function call from auipc and jalr to jal.

Could you please add tests that demonstrate the new logic in shouldForceRelocation? Specifically, it would be good to show that relocations are emitted even for local control flow, when they weren't before.

Thu, Apr 5, 3:48 AM
asb added a comment to D44888: [RISCV] Default enable linker relaxation and add -mrelax, -mno-relax flags.
In D44888#1058257, @asb wrote:

Could you please add a test? Given that the current version of D44886 enables linker relaxation by default in the backend, shouldn't -mno-relax cause -relax to be set?

Thu, Apr 5, 3:36 AM
asb added a comment to D44888: [RISCV] Default enable linker relaxation and add -mrelax, -mno-relax flags.

Could you please add a test? Given that the current version of D44886 enables linker relaxation by default in the backend, shouldn't -mno-relax cause -relax to be set?

Thu, Apr 5, 3:33 AM
asb added inline comments to D44886: [RISCV] Support linker relax function call from auipc and jalr to jal.
Thu, Apr 5, 3:24 AM
asb added a comment to D44885: [RISCV] Expand function call to "call" pseudoinstruction.

Thanks for this as always Shiva. Mostly minor comments or questions in this review.

Thu, Apr 5, 2:47 AM
asb added a comment to D42780: [RISCV] CompressPat Tablegen-driven Instruction Compression.

Thanks for the test changes.

Thu, Apr 5, 1:59 AM
asb added a comment to D45284: [RISCV] More validations on the input value of -march=.

Based on Andrew's response (thanks Kito for sending the query) it looks like GCC accepting lowercase only is intentional, and we should follow that. In which case, it might be an improvement to reject uppercase letters in the ISA string with a message saying that only lowercase letters are accepted.

Thu, Apr 5, 1:34 AM
asb requested changes to D45237: [RISCV] Fix logic to check if frame pointer should be used.
Thu, Apr 5, 1:29 AM

Wed, Apr 4

asb added inline comments to D45284: [RISCV] More validations on the input value of -march=.
Wed, Apr 4, 1:56 PM
asb added a comment to D42780: [RISCV] CompressPat Tablegen-driven Instruction Compression.
In D42780#1039073, @asb wrote:

Looking at RISCVGenCompressInstEmitter:

  • I'm not sure I see the need for the RISCVAreEqualOperands helper. You already don't consistently check that every operand isReg. I think it's fine to rely on invalid operands at this stage being a programming error which would be picked up in an asserts build by getReg presumably.

This function is added to check for tied operands, weather it being a register or immediate. for example , if you use the patters (INSTR $rs $rs ) we need a check that these two operands are equal in value (if resisters, hold the same register, if immediates hold the same immediate) . Currently RISCV doesn't have timed immediate operands but this backend is written keeping in mind that new patterns or different ISAs might use it.

so in case Immediate check is need this function should be updated to:
return ( MCOp1.isReg() && MCOp2.isReg() &&

   (MCOp1.getReg() == MCOp2.getReg())  ) || 
( MCOp1.isImm() && MCOp2.isImm() &&
   (MCOp1.getImm() == MCOp2.getImm())

What places do you think we should have checked for an operand being a register? the combination of validation time (compile the td file) and runtime check should catch all cases I think. Maybe we are missing something?

Wed, Apr 4, 8:47 AM
asb accepted D44727: [RISCV] Extend getTargetDefines for RISCVTargetInfo.

Thanks, this looks good to me.

Wed, Apr 4, 8:31 AM
asb added a comment to D42780: [RISCV] CompressPat Tablegen-driven Instruction Compression.
In D42780#1023507, @asb wrote:

Hi Sameer. Some notes primarily from reviewing the patterns and testing so far:

  • In the RISCV backend we order tablegen instruction definitions to match the ordering in the instruction set manual (which allows easy cross-reference, makes it harder to miss one) and typically order codegen patterns grouped by function. For the case of the CompressPat, I'd probably keep them ordered by the instruction set manual (page 82 in the spec, like the instruction definitions) as each pattern is so mechanical. This makes it easier at a glance to see that every compressed instruction has a matching transformation. The basic MC tests should be ordered to reflect this too. I know this is probably a tedious change to make, but I do think a little effort in the formatting here has an impact on maintainability.

Thank you for the comment, Alex.

Th problem with this is that the instruction are not ordered base on the required target features, C.FLW is an rv32 only instruction but C.LD is rv64/128 then followed by C.FSD. This causes the listing to be full of
let Predicates = [HasStdExtC] in {
}
if we don't group the instructions based on target features. I think this will make the listing a bit messy, what do you think?

Wed, Apr 4, 8:24 AM
asb added a comment to D44727: [RISCV] Extend getTargetDefines for RISCVTargetInfo.

Thanks Eli for the info on hasFeature.

Wed, Apr 4, 7:58 AM
asb added a comment to D45237: [RISCV] Fix logic to check if frame pointer should be used.

Could you please add a test, perhaps to test/Driver/frame-pointer.c?

Wed, Apr 4, 7:55 AM
asb added a comment to D45119: [RISCV] Override EmitToStreamer in RISCVAsmPrinter to handle missed compression opportunities.

This seems like a good improvement. As Eli says, avoiding the virtual function if possible would be ideal.

Wed, Apr 4, 7:44 AM

Thu, Mar 29

asb accepted D41932: [RISCV] Hooks for enabling instruction compression.

This diff seems to have accidentally picked up changes to a number of tests.

Thu, Mar 29, 1:03 AM

Wed, Mar 28

asb added a comment to D43501: [lit] Implement 'cat' command for internal shell.

Could you please make a new review with your proposed fix(es) so we can continue discussion there? It's much easier to keep track of things that way. Typically one phabricator review == one commit.

Wed, Mar 28, 8:11 AM
asb added a comment to D43501: [lit] Implement 'cat' command for internal shell.

Could you please post the change as a separate revision, as a diff vs what was committed?

Wed, Mar 28, 12:00 AM

Mar 23 2018

asb added a comment to D41949: [RISCV] implement li pseudo instruction.
In D41949#1045516, @asb wrote:

I haven't looked into it more closely, but I do note a minor codegen change for float-mem.ll which results in an extra instruction:

Good catch, I missed that codegen change. Seems like the ADDI was previously merged into the FLW. Given that we can solely use PseudoLI for constants we probably only miss a simplification pattern or a simple peephole optimisation. I can have a look but given that I currently do not use floating point instructions it may take some time.

Mar 23 2018, 7:37 AM

Mar 22 2018

asb accepted D41949: [RISCV] implement li pseudo instruction.

Thanks Mario. I think this is looking good to land now.

Mar 22 2018, 5:37 AM
asb updated subscribers of D44727: [RISCV] Extend getTargetDefines for RISCVTargetInfo.

Thanks Kito. I've added some comments inline.

Mar 22 2018, 5:02 AM
asb accepted D44189: [RISCV] Verify the input value of -march=.

Thanks for this Kito. A tiny formatting nit, but otherwise this looks good to me.

Mar 22 2018, 4:22 AM
asb added inline comments to D41932: [RISCV] Hooks for enabling instruction compression.
Mar 22 2018, 3:51 AM
asb accepted D44750: [RISCV] Use init_array instead of ctors for RISCV target, by default.

Hi Mandeep, thanks for the patch. This looks good to me, but there are just two changes I would suggest prior to committing:

Mar 22 2018, 3:39 AM

Mar 21 2018

asb committed rL328104: [RISCV] Codegen support for RV32F floating point comparison operations.
[RISCV] Codegen support for RV32F floating point comparison operations
Mar 21 2018, 8:13 AM
asb committed rL328102: [RISCV] Add tests missed from r327979.
[RISCV] Add tests missed from r327979
Mar 21 2018, 7:53 AM

Mar 20 2018

asb committed rL327979: [RISCV] Add codegen for RV32F floating point load/store.
[RISCV] Add codegen for RV32F floating point load/store
Mar 20 2018, 6:30 AM
asb committed rL327976: [RISCV] Add codegen for RV32F arithmetic and conversion operations.
[RISCV] Add codegen for RV32F arithmetic and conversion operations
Mar 20 2018, 5:48 AM

Mar 19 2018

asb committed rL327831: [RISCV] Peephole optimisation for load/store of global values or constant….
[RISCV] Peephole optimisation for load/store of global values or constant…
Mar 19 2018, 4:58 AM

Mar 15 2018

asb added a comment to D42780: [RISCV] CompressPat Tablegen-driven Instruction Compression.
In D42780#1038674, @asb wrote:

By the way, I'm getting the following error while trying to build this:

CMake Error: The inter-target dependency graph contains the following strongly connected component (cycle):
  "LLVMRISCVAsmPrinter" of type SHARED_LIBRARY
    depends on "LLVMRISCVDesc" (weak)
  "LLVMRISCVDesc" of type SHARED_LIBRARY
    depends on "LLVMRISCVAsmPrinter" (weak)
At least one of these targets is not a STATIC_LIBRARY.  Cyclic dependencies are allowed only among static libraries.

If you're not seeing this, it may be because I build with -DBUILD_SHARED_LIBS=true during development.

Mar 15 2018, 10:12 AM
asb added a comment to D42780: [RISCV] CompressPat Tablegen-driven Instruction Compression.

By the way, I'm getting the following error while trying to build this:

Mar 15 2018, 7:21 AM

Mar 14 2018

asb added a comment to D44189: [RISCV] Verify the input value of -march=.

Thanks for submitting this Kito. I've added some minor in-line comments. It might also be worth adding a couple of extra cases to the tests:

  • Repeated letters in the ISA string (e.g. rv32immafd)
  • Upper case letters in the ISA string. We currently reject these (as does GCC). It would be worth having a test that tracks this behaviour
Mar 14 2018, 1:34 PM
asb added a comment to D43158: [RISCV] Always emit a symbol for R_RISCV_PCREL_LO12_I..

Thanks for pursuing this issue on the binutils bug tracker. Might you be interested in proposing a patch to the psabi doc that clarifies the requirement here? It seems that the main reason for forcing this behaviour is for linker relaxation - I hope we can co-ordinate with binutils to allow addends when linker relaxation is disabled.

Mar 14 2018, 10:25 AM
asb added a comment to D43157: [RISCV] Properly evaluate VK_RISCV_PCREL_LO.

And I have to apologise again that it has taken longer than expected to get back to you on this. I hope my slow feedback this time won't put you off future contributions - going forward, things should be much faster.

Mar 14 2018, 10:19 AM
asb added a comment to D44244: [LLVM] Add -git-commit-after-all option.

For reference, this was discussed on llvm-dev here: http://lists.llvm.org/pipermail/llvm-dev/2018-March/121634.html

Mar 14 2018, 9:30 AM
asb accepted D43752: [RISCV] Preserve stack space for outgoing arguments when the function contain variable size objects.
Mar 14 2018, 9:26 AM
asb added a comment to D43752: [RISCV] Preserve stack space for outgoing arguments when the function contain variable size objects.

Thanks for making those changes Shiva . The fact we can simplify determineFrameLayout is a good observation.

Mar 14 2018, 9:23 AM

Mar 13 2018

asb added a comment to D44153: Build system changes for RISCV.
In D44153#1031976, @asb wrote:

Actually, looking again I'm not having any problems building just the RISCV backend even without this patch. e.g. an invocation like the following seems fine:

cmake -G Ninja -DCMAKE_BUILD_TYPE="Debug" \
  -DBUILD_SHARED_LIBS=True -DLLVM_USE_SPLIT_DWARF=True \
  -DLLVM_OPTIMIZED_TABLEGEN=True \
  -DLLVM_BUILD_TESTS=True \
  -DLLVM_TARGETS_TO_BUILD="RISCV" \
  -DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD="RISCV" ../
cmake --build .

Could you please clarify exactly which build problems this fixes?

Alex, add -DLLVM_TARGET_ARCH=riscv32-unknown-linux-gnu to your cmake command above, and you'll hit the "Unknown architecture ..." error. Since we are cross-compiling, we'll need that option to default to riscv target arch. Without that option, it will target the host as the native arch.

Mar 13 2018, 12:18 PM

Mar 8 2018

asb added a comment to D44153: Build system changes for RISCV.

Actually, looking again I'm not having any problems building just the RISCV backend even without this patch. e.g. an invocation like the following seems fine:

Mar 8 2018, 2:11 PM
asb accepted D44153: Build system changes for RISCV.

Hi Azharuddin, many thanks for the contribution. I hadn't spotted this part of LLVM's CMake build system, and it's definitely useful to be able to build just the RISCV backend. I've tried this locally, and the problem I'm seeing is that a good number of Clang and LLVM tests fail when you don't build X86 support. On my system, 351 unexpected failures across Clang+LLVM. This is a problem with the tests rather than this patch (they should really be disabled if there is no X86 support), but it does limit the usefulness of just building _only_ the RISC-V backend. Is it expected that you build an LLVM with X86 and RISCV, ensure 100% tests pass, then build with just the RISCV backend and ignore any new failures?

Mar 8 2018, 2:02 PM

Mar 2 2018

asb accepted D43055: [RISCV] Implement MC relaxations for compressed instructions..

Looks good to me. You might want to use c.nop in the tests rather than nop to avoid test churn (changing offsets) once automatic compression is merged.

Mar 2 2018, 9:18 AM
asb accepted D43328: [RISCV] Update MC compression tests.

Now this is decoupled from the other changes, it seems good to land whenever you're ready.

Mar 2 2018, 9:12 AM
asb added a comment to D43752: [RISCV] Preserve stack space for outgoing arguments when the function contain variable size objects.

Hi Alex. Default hasReservedCallFrame will return !hasFP(), so these cases will generate extra sp adjustment, we could override hasReservedCallFrame as !hasVarSizedObjects() to avoid this and thanks for update the test case which make it more clean and clear!

Mar 2 2018, 2:55 AM

Mar 1 2018

asb added a comment to D43752: [RISCV] Preserve stack space for outgoing arguments when the function contain variable size objects.

The extra sp adjustments introduced in calling-conv.ll and vararg.ll seem unnecessary - are they?

Mar 1 2018, 6:07 AM
asb added a comment to D42780: [RISCV] CompressPat Tablegen-driven Instruction Compression.

Hi Sameer. Some notes primarily from reviewing the patterns and testing so far:

  • In the RISCV backend we order tablegen instruction definitions to match the ordering in the instruction set manual (which allows easy cross-reference, makes it harder to miss one) and typically order codegen patterns grouped by function. For the case of the CompressPat, I'd probably keep them ordered by the instruction set manual (page 82 in the spec, like the instruction definitions) as each pattern is so mechanical. This makes it easier at a glance to see that every compressed instruction has a matching transformation. The basic MC tests should be ordered to reflect this too. I know this is probably a tedious change to make, but I do think a little effort in the formatting here has an impact on maintainability.
  • Related to the above, grouping the 'commuted and repeated patterns' after the normal patterns makes it easier to miss one and a little more effort to check it's there. I'd always group patterns for the same target compressed instruction together.
  • Are the CHECK-BYTES lines worthwhile? I'm fairly happy making the assumption that the encoding printed by the InstPrinter is reflected in the output object (or at least, I'm not sure it's worth adding an extra line for every single check to ensure this)
  • For other MC tests, we've checked the instructions common to rv32 and rv64 in a single file. See e.g. rv32c-valid.s, rv32c-only-valid.s etc
  • This compression transformation is intended to use the same code path to support a range of use cases: .s->.o, .ll->.o, showing aliases in disassembly, compression of inline asm. This patch only contains tests for the assembler and disassembler paths. Given that the compression transformation is implemented as an MCInst->MCInst transformation, it's fine for the exhaustive tests to live in test/MC/RISCV, but we should at least have some sanity checks that the codegen compression path is working (including with inline asm).
Mar 1 2018, 3:42 AM
asb added a comment to D43055: [RISCV] Implement MC relaxations for compressed instructions..

Thanks for adding the test case, I added a few minor comments there.

Mar 1 2018, 2:37 AM
asb added inline comments to D43328: [RISCV] Update MC compression tests.
Mar 1 2018, 12:33 AM

Feb 28 2018

asb added inline comments to D41932: [RISCV] Hooks for enabling instruction compression.
Feb 28 2018, 6:20 PM
asb updated subscribers of D41932: [RISCV] Hooks for enabling instruction compression.

Adding llvm-commits as a subscriber. A couple more comments inline

Feb 28 2018, 6:17 PM
asb committed rL326309: [RISCV] Update two tests after r326208.
[RISCV] Update two tests after r326208
Feb 28 2018, 12:23 AM