Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cheri LLVM-16 #742

Open
1 of 2 tasks
veselypeta opened this issue Jun 24, 2024 · 20 comments
Open
1 of 2 tasks

Cheri LLVM-16 #742

veselypeta opened this issue Jun 24, 2024 · 20 comments

Comments

@veselypeta
Copy link

veselypeta commented Jun 24, 2024

Hi. I've been working on rebasing the compiler to LLVM-16. Unfortunately the changes are too big to raise as a PR so @arichardson recommend raising an issue here instead. For testing I've got all the clang/lld/llvm test suites passing (perhaps there are others) and I've managed to build and run CheriBSD purecap.

Branch: https://github.com/veselypeta/cherillvm/tree/petr/llvm-17
Target Commit: b0daacf (one before llvmorg-17-init tag)

Things still to do:

  • getRegAllocHints : I've disabled this for CHERI. It hits an assertion when trying to use CHERI registers. I've not looked into this deeply, but it hits an assertion when it checks that it's not got a subreg.
  • Convert all CHERI tests to opaque pointers. I've had to modify a number of tests with -opaque-pointers=0 to force the old behaviour. However, these should all now be updated to opaque pointers as of llvm-16.

To build CheriBSD I had to use an updated version of libcxx. To Build

I'm happy to help and provide more testing or descriptions of the changes etc. Thanks.

@arichardson
Copy link
Member

I had to apply the following patch to build and run tests:

diff --git a/llvm/unittests/CodeGen/TestAsmPrinter.h b/llvm/unittests/CodeGen/TestAsmPrinter.h
index 47ef75cbada9..db79913b17ce 100644
--- a/llvm/unittests/CodeGen/TestAsmPrinter.h
+++ b/llvm/unittests/CodeGen/TestAsmPrinter.h
@@ -30,9 +30,9 @@ public:

   MOCK_METHOD2(emitSymbolAttribute,
                bool(MCSymbol *Symbol, MCSymbolAttr Attribute));
-  MOCK_METHOD3(emitCommonSymbol,
+  MOCK_METHOD4(emitCommonSymbol,
                void(MCSymbol *Symbol, uint64_t Size, Align ByteAlignment, TailPaddingAmount TailPadding));
-  MOCK_METHOD5(emitZerofill,
+  MOCK_METHOD6(emitZerofill,
                void(MCSection *Section, MCSymbol *Symbol, uint64_t Size,
                     Align ByteAlignment, TailPaddingAmount TailPadding, SMLoc Loc));

There are two tests failing as well as a bunch of llvm-exegesis ones since I skipped that in my build and it no longer omits those.

The getRegAllocHints() crash reduces down to:

; RUN: llc -o /dev/null -mtriple=riscv64-unknown-freebsd14.0 -relocation-model=pic -mattr=+m -mattr=+a -mattr=+f -mattr=+d -mattr=+c -mattr=+xcheri -mattr=-relax -mattr=+cap-mode -mattr=-save-restore -target-abi l64pc128d -vectorize-loops -vectorize-slp -mcpu=generic-rv64 -O2 -treat-scalable-fixed-error-as-warning -verify-machineinstrs %s
target datalayout = "e-m:e-pf200:128:128:128:64-p:64:64-i64:64-i128:128-n32:64-S128-A200-P200-G200"
target triple = "riscv64-unknown-freebsd14.0"

; Function Attrs: nounwind willreturn memory(none)
declare i64 @llvm.cheri.cap.address.get.i64(ptr addrspace(200)) addrspace(200) #0

define linkonce_odr ptr addrspace(200) @_ZN9libunwind10CFI_ParserINS_17LocalAddressSpaceEE9decodeFDEERS1_u11__uintcap_tPNS2_8FDE_InfoEPNS2_8CIE_InfoEb(ptr addrspace(200) %addressSpace) addrspace(200) {
entry:
  %0 = call i64 @llvm.cheri.cap.address.get.i64(ptr addrspace(200) %addressSpace)
  %add58 = add i64 1, %0
  store i64 %add58, ptr addrspace(200) null, align 8
  ret ptr addrspace(200) null
}

attributes #0 = { nounwind willreturn memory(none) }

The "Cheri Get Address Elimination" pass removes the PsedoCGetAddr and converts it to a ADDI on a subreg (which makes sense), but it seems something later one has broken.

@veselypeta
Copy link
Author

Thanks :). I've applied your patch. How do I reproduce your test failure? So far I've been running the check-all target, but can't see any failures.

@arichardson
Copy link
Member

Regarding the other test failures, that is probably because I am building with cheribuild.py llvm --build-and-test (--clean) and it defaults to not building tools that we don't use. I've had to update some tests that assume presence of all tools in past so this is likely just one more of these. Should really upstream this...

@veselypeta
Copy link
Author

Thanks Alex: the issues were 2 fold

  • A mismerged .cfg file, which included the expections for llvm-exegesis not being built
  • this commit, which added MCA support to RISCV target. I've disabled it if the LLVM_TOOL_LLVM_MCA_BUILD=False option is specified.

@arichardson
Copy link
Member

I've pushed some follow-up fixes to https://github.com/arichardson/llvm-project/tree/llvm-16-test where I've marked the commit the fix should be folded into. @jrtc27 not sure if that is something you would want to fix with your rebase-with-merges script?

@veselypeta
Copy link
Author

Any update on this? Happy to help resolve any issues.

@arichardson
Copy link
Member

I finally got around to doing some testing with the latest version from https://github.com/arichardson/llvm-project/tree/llvm-16-test and it seems to work just fine with CheriBSD releng/23.11 with a few minor makefile changes to disable new warnings.

@jrtc27 would you be okay with pushing this to upstream-llvm-merge after folding the fixup commits into the correct initial commit? I haven't used your script yet so not sure how long it will take me to get it right but I could give it a try unless you are able to do so.

@jrtc27
Copy link
Member

jrtc27 commented Jul 25, 2024

I can take a look next week when I’m back from annual leave

@veselypeta
Copy link
Author

Ideally we would like to proceed with upgrading the version further to llvm-17. Would it cause problems to begin using alex's branch as a base or should we wait until it lands in the repo?

@arichardson
Copy link
Member

Ideally we would like to proceed with upgrading the version further to llvm-17. Would it cause problems to begin using alex's branch as a base or should we wait until it lands in the repo?

I think we should be almost ready for this - just finishing up cleaning the history.

@arichardson
Copy link
Member

I have started the final rebase since I made a mistake in the last one and should be able to push it once it has completed in around 2 hours.

@arichardson
Copy link
Member

I have pushed a version with the history cleaned up to https://github.com/CTSRD-CHERI/llvm-project/tree/upstream-llvm-merge.

@arichardson
Copy link
Member

@veselypeta FYI I just pushed a new history (same contents) to https://github.com/CTSRD-CHERI/llvm-project/tree/upstream-llvm-merge. I think once CI and CheriBSD is happy with this we can merge it to dev.

@jrtc27
Copy link
Member

jrtc27 commented Aug 2, 2024

Perhaps worth posting a summary of what needed cleaning up to hopefully avoid the need in future?

@veselypeta
Copy link
Author

Thank you for your help @arichardson. Yeah any advice/feedback would be much appreciated.

@arichardson
Copy link
Member

Overall this is what changed: I folded some compilation fixes into the commit where the failure would have started to make bisecting more likely to work correctly. I also dropped the changes to disable the riscv regalloc changes by moving the fix after the change that introduced the assertion. Finally, a few smaller cleanups of commit messages. Overall this was about 10-15 commits that I moved around.

So in summary nothing major, just trying to keep the history in a "mostly" bisectable state.

@arichardson
Copy link
Member

This is not somethign that needs to be done while performing merges - the fixes whenever you stop merging and test compilation is the right approach.

Having lots of small commits to fix each individual compilation issue made it a lot easier to fold the fixup into/after the upstream commit that introduced the problem was great, so I did not have to split up your commits. Thanks for all your hard work performing this merge!

@jrtc27
Copy link
Member

jrtc27 commented Aug 13, 2024

I've been combing through the merge commits, discussing with Alex at points, and am finding various issues, some quite important (in particular, libunwind looks utterly broken for RISC-V and CHERI-RISC-V due to multiple cumulative mismerges such that it shouldn't even build, but definitely won't work properly on CHERI-RISC-V).

Without listing all the specifics that I've found so far, the general issues I've found multiple instances of are:

  • Adding trailing whitespace
  • Removing upstream's trailing whitespace (yes, it's a "fix", but those are annoying diffs to carry that serve no use)
  • Mentioning your commits by hash; this means any rebasing that gets done prior to reaching dev breaks the link. Use the upstream hash instead, unless you really need to reference the merge itself, which should be rare, that's normally for referring to a mismerge done years ago.

Once I've reached the end I'll do another big rebase to fold the fixes into the relevant commits (and reword commit messages that reference rebased merges), but reviewing it all is taking a while and rebasing that large a history can be slow and fiddly.

@veselypeta
Copy link
Author

Thanks for looking into this. I only noticed the issues with libunwind until after I had completed the merge. I have a patch to fix the build of libunwind, but it probably won't help you trying to clean up the history of the mismerges. Thanks for your advice about doing the merges, I'll keep that in mind going forward. I appreciate that I didn't keep the history very clean, especially at the start or the merge.

@arichardson
Copy link
Member

Thanks for looking into this. I only noticed the issues with libunwind until after I had completed the merge. I have a patch to fix the build of libunwind, but it probably won't help you trying to clean up the history of the mismerges. Thanks for your advice about doing the merges, I'll keep that in mind going forward. I appreciate that I didn't keep the history very clean, especially at the start or the merge.

I have pushed the fixes for libunwind on top of the latest upstream-llvm-merge branch. We will clean up the history one more time, but that should not be a blocker for your continued merge - it can be rebased on top of the latest upstream-llvm-merge branch once completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants