[PhilippvK]

I ran into these issues using +xcvbi in TVM's LLVM backend. The patches are minimal but I am not sure how to add a unit test for this bugfix.

[lewis-revill]

Fix looks good! Since we are probably going to squash this into the xcvbi commits as a fixup when upstreaming, it wouldn't make sense to have a specific regression test for this in that case. But if you can provide some way of reproducing this issue as a separate issue in the issue tracker and link this PR as the fix - just some IR output from the frontend that causes the crash. Otherwise this is good to merge, if the person who merges this is able to write a descriptive commit message when merging.

[PhilippvK]

I have to convert this into a Draft PR because I ran into a few more bugs related to the immediate Branching instructions.

• It seems like using clang the cv.beqimm and cv.bneimm will only be inserted by LLVM with optimizations disabled (-O0) while for some reason clang foo.c -O3 -S -emit-llvm ... -> llc foo.ll -O3 ... -> clang foo.s -O3 ... seems to work.
• If immediate branching instructions are present, it can happen that their mnemonic will be replaced with beq or bne by the control flow optimization pass. This does then result in invalid assembly code since the register-register branching instructions can not deal with the additional immediate operand. The main problem is that when replacing the Branch Maching Instructions (in RISCVInstrInfo::insertBranch) a reverse lookup solely based on the condition code (CC) is performed without checking the types of the Cond. c6a9e81 contains a workaround for that but I have the feeling, that there might be further patches required.

Here are some preliminary benchmark results with the workarounds implemented. Only up to 1% speedup (in dynamic instruction count) can be obtained here.

[PhilippvK]

Proposal for unit test can be found here: #67 (comment)

[PhilippvK]

Since the upstreaming of xcvbi is already in progress (https://reviews.llvm.org/D154412) and this fixes major bugs (segfaults) which almost everyone using -O2 or higher will run into. Wouldn't this patch need to be applied on the upstream as well before a merge?

[melonedo]

Since the upstreaming of xcvbi is already in progress (https://reviews.llvm.org/D154412) and this fixes major bugs (segfaults) which almost everyone using -O2 or higher will run into. Wouldn't this patch need to be applied on the upstream as well before a merge?

No. Only encoding (Machine Code) is included in D154412, while this PR affects CodeGen, which is planned be submitted to LLVM after the encoding of all patches of encoding (in #65) are finished. So this PR will not immediately affect D154412 AFAICT.

[PhilippvK]

@melonedo Oh, I missed that. Then a merge of this is not so urgent. Sorry for the confusion.

[jeremybennett]

@PhilippvK Thanks for this. Do you wish to resolve the remaining questions, so we can get this merged. Thanks

[PhilippvK]

Sorry for the delay. I will add the proposed unit tests today or over the weekend. After that it should be ready to merge.

[PaoloS02]

I've tested this pull request on top of the latest development branch and the macOS tests pass. So it's good to merge. It would be good to have the commits squashed into fewer or one commit if possible as some of the commits are just about fixing the indentation.

[PhilippvK]

Rebased on top of upstream/development and squashed the changes into two commits. After CI passes, you can merge this.