This isn't part of LLVM I usually hack on, but I just wanted to comment to say:

1. I've manually verified this patch applies cleanly to top of tree and all checks pass
2. The patch looks good to me, and addresses the final nits from @arphaman in the previous revision. The only other changes vs the previous revision seems to be the removal of CurrentNode from the initialiser list as the field now has a default initialiser (as of rL305969).

Patch committed, many thanks for your contribution @chenwj.

Thank you @chenwj, this is definitely an improvement over the current documentation. Would you like me to commit?

Hi, thanks for the patch. My feedback is:

1. Limitations of literal formats shouldn't be discussed when introducing the types.
2. Perhaps outside the scope of this patch, but it does seem a little odd that a simple string literal can be assigned to either a string or code variable, but code fragment literals can't be assigned to string variables (at least on the couple of months old build on LLVM I have on this machine). The existing description that a code fragment is "just a multiline string literal" seems incorrect due to this.

Looks good to me.

Phew, this is a monster patch to review but clearly there's no real way it could be reduced further. I have no real experience with the TableGen implementation, which limits my ability to contribute to this code review. I have at least carefully read through the code to try to spot any obvious logic errors or issues.

Thanks, looks good to me.

I had a couple of minor review comments. I know the use of pointers vs references is quite inconsistent across LLVM, but I do feel it would be an improvement if isLegalRC took a const TargetRegisterInfo& (though perhaps there's a reason not to do this that I'm missing?).

Reference to previous related discussion: D24631 and http://lists.llvm.org/pipermail/llvm-dev/2016-September/105027.html

Refreshed the patch for current HEAD and double-checked all tests still pass. I'd really appreciate a review so we can get this committed.

I've just verified this patch still applies cleanly to HEAD and, as before, all tests pass. Any comments? I think this is a useful cleanup for backends that already report errors for out-of-range fixups, and will make it easier to improve other backends with similar error reporting.

@nemanjai: MachineInstr::setFlag will do Flags |= Flag, so any previously set flags are maintained.

Hi all. Sorry for digging up an old review, but I've been looking at reporting fixup overflow errors for RISC-V, and this seems the most appropriate place to discuss how it's currently handled in AArch64. I agree with @rengolin's concerns with this approach - or at least, I think we can do this better. Right now, adjustFixupValue is always called twice. The second time it's called, the MCContext argument is null and the code relies on the fact that any errors should have been caught the first time round, and so this null pointer will never be dereferenced [EDIT: sorry, that's not true - it does consistently check that Ctx is non-null]. I like having error checking alongside the value adjustments, but I propose that instead we achieve this by:

The previous version of this patch made the mistake when decoding a GPR of assuming you can use the parsed RegNo as an index into the register class. In the general case, a register class will have the registers in preferred allocation order, meaning this will fail. Instead, we have a GPRDecoderTable to select the appropriate register based on the parsed RegNo.

Here's the same patch as before with full context, apologies for the noise.

The diff I attached a few hours ago didn't include all context, this update fixes that. Sorry for the noise.

This refresh adds support for relocations and fixups on branch and jal instructions. This is sufficient for compiling simple programs.

Refresh patch and incorporate suggestion from @jyknight regarding FENCE and FENCEI (thanks!).

Update to address review comment (just use StringRef rather than introducing Token struct).

I think that, as you point out, it will depend on what sort of changes you're making. The use of multiclasses etc do sometimes make it more difficult to jump in and make a small change, but also give you power to make changes across a large number of definitions at once.

Hi Krzysztof - is the tool that generated these instruction definitions open source, or at least available somewhere? I know that given Hexagon's current development pattern this is more of a theoretical concern than a practical one, but it does seem a barrier to people preparing patches for these .td files.

@dberlin, @davide: Perhaps this is my confusion rather than anything else, but should it read "The `llvm.ssa_copy` intrinsic can be used to attach information to operands" rather than to operations?

In D23568#663066, @Razer6 wrote:

@theraven @jyknight What's the state of this change?

In D23563#639241, @grosser wrote:

Hi Alex,

I silently followed the RISC-V patches being upstreamed and did not see an update here for a little while. Is this patch blocked on something or did you just not get to this one yet?

Closed by rL285708 and rL285709.

Update test style as suggested by @jyknight in D23564

Fix rv32i-valid.s, so CHECK-INST is used rather than CHECK-DIS.

Update test style as suggested by @jyknight in D23564

Update test style as suggested by @jyknight

Refresh to follow changes made in rL283702, putting TheRISCV32Target and TheRISCV64Target behind accessors.

Refresh to follow changes made in rL283702, putting TheRISCV32Target and TheRISCV64Target behind accessors. Additionally, drop the unused getEndLoc after rL283691 removed it from MCParsedAsmOperand

Refresh to follow changes made in rL283702, putting TheRISCV32Target and TheRISCV64Target behind accessors.

Refresh patch to follow the changes made to other backends in rL283702, moving TheRISCV32Target and TheRISCV64Target behind an accessor function.

In D25407#566074, @grimar wrote:

Hi Brian, I have a error after that change on windows (when reverting r283710, it is works fine again):

C:\c_make_build_dir\Debug\bin\llvm-lit.py -v C:\llvm\test\Object\invalid.test
Traceback (most recent call last):

  File "C:\c_make_build_dir\Debug\bin\llvm-lit.py", line 44, in <module>
    lit.main(builtin_parameters)
AttributeError: 'module' object has no attribute 'main'

Fix the issue described in my previous comment by not defining an AsmName and AltName for the RISCV64 registers. With this approach, the generated MatchRegisterName and MatchAltRegisterName functions can be used. In the future, a little bit of extra logic in RISCVAsmParser.cpp can be added to coerce a parsed register from 32-bit to 64-bit when desired.

The last update of this patch was incomplete. Now fixed.

Rather than the hacky previous approach (unconditionally emittingthe enum), move it to a header.

It only shows up upon a clean compile (CMake issue?), but I've just found the introduction of registers with the same assembly name now leads to an assertion during compilation within llvm-tblgen. I'll investigate a fix.

Refresh patch to reflect changes earlier in the series.

Tests now check both assembly printing and disassembly. I've added a comment/TODO regarding decode of instructions outside of the RV32/RV64 core that might be 16-bit, 48bit, 64bit, ...

Make use of isShiftedInt from MathExtras.h. Rename {simm21,simm13}_mask1 to {simm21,simm13}_lsb0. Tests are updated to check instruction printing.

Tests have been modified to check instruction printing.

Address review comments from @jyknight and @t.p.northover. Also use # rather than !strconcat in RISCVInstrInfo.td

I have updated RISCVRegisterInfo.td so it defines both the 32-bit and 64-bit GPRs, marking the 32-bit GPRs as subregs. As explained in comments, the hope is that we can later move to using something like D24631.

Hi Krzysztof. So as discussed in D23561 the motivating problem for this work is cases (as in Hexagon HVX) where instructions with identical encodings but different RegisterClasses currently need to be defined twice. An example of this is valignb. Having a register class with a non-constant register size and alignment would solve the issue for HVX, but there's also the hope it will be useful for other targets. For these HVX instructions there is no list<dag> pattern defined. If a pattern was specified, surely even with this new functionality you'd need to have repeated instruction definitions in order to define multiple patterns, because they would need different ValueTypes? Do you have something in mind that would address this as well?

@jyknight: Yes, that's a good point, once codegen is added I'll need to thread through a ValueType in a similar way to how I currently pass RegisterClass. I can definitely see the argument that describing instructions twice and abstracting later might lead to the best solution. I think it's definitely worth considering the alternatives early on though, even if I do go ahead with the duplication approach.

Apologies for the slight delay, organising last week's LLVM Cauldron and other activities in my life had limited my time.

Fix whitespace damage (thanks @emaste!)

Address review comment from @theraven

Rename ProcessorModels to generic-rv32 and generic-rv64 and fix incorrect immediate size spotted by @jordy.potman.lists