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

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.

In D45284#1074282, @apazos wrote:

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.

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.

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

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'

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?

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)

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

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

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.

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.

In D44887#1069639, @shiva0217 wrote:

In D44887#1069614, @asb wrote:

In D44887#1069573, @shiva0217 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?

In D44887#1069573, @shiva0217 wrote:

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.

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

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.

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.

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.

Thanks! Looks good to me.

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.

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.

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

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

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).

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

Thanks, looks good to me.

Thanks Kito, looks good to me.

Thanks, this looks good to me now.

Looks good to me. Thanks.

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

Thanks for this Simon.

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.

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?

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?

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

Thanks for the test changes.

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.

In D42780#1050057, @sabuasal wrote:

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?

Thanks, this looks good to me.

In D42780#1048737, @sabuasal wrote:

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?

Thanks Eli for the info on hasFeature.

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

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

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

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.

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

In D41949#1046704, @niosHD wrote:

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.

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

Thanks Kito. I've added some comments inline.

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

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

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.

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

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

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.

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.

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

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

In D44153#1036096, @azharudd wrote:

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.

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:

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?

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.

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

In D43752#1024857, @shiva0217 wrote:

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!

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

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).

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

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