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][InstCombine] Fix 8-byte capability memcpy inlining #736

Merged
merged 2 commits into from
May 16, 2024

Conversation

StephenHuwClarke
Copy link

The motivating case for this change is https://cheri-compiler-explorer.cl.cam.ac.uk/z/ore1Ej - the compiler is copying a capability using non-cap load and store. The issue is for copying 8-byte capabilities only, so won't affect targets with 16-byte capabilities e.g. rv64.

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp Outdated Show resolved Hide resolved
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp Outdated Show resolved Hide resolved
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp Outdated Show resolved Hide resolved
; CHECK-NEXT: store ptr addrspace(200) [[TMP0]], ptr addrspace(200) [[D:%.*]], align 8
; CHECK-NEXT: ret ptr addrspace(200) [[D]]
;
entry:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need to name the blocks

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fixed now?

; CHECK-NEXT: ret ptr addrspace(200) [[D]]
;
entry:
; memcpy can be transformed to capability load/store
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use ;; for non-FileCheck comments, and put at the top of the test function rather than buried inside it

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fixed now?

ret ptr addrspace(200) %d
}

; Function Attrs: argmemonly mustprogress nocallback nofree nounwind willreturn
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useless comment from original IR dump

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fixed now?

}

; Function Attrs: argmemonly mustprogress nocallback nofree nounwind willreturn
declare void @llvm.memcpy.p200.p200.i32(ptr addrspace(200) noalias nocapture writeonly, ptr addrspace(200) noalias nocapture readonly, i32, i1 immarg) addrspace(200) #1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even all these attributes should be removable, LLVM will add them automatically

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fixed now?

ret ptr addrspace(200) %d
}

define ptr addrspace(200) @test_memcpy8_nocap_align_unkown(ptr addrspace(200) noundef %d, ptr addrspace(200) noundef %s) local_unnamed_addr addrspace(200) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo ("unkown")

@jrtc27
Copy link
Member

jrtc27 commented May 15, 2024

Your second commit is authored by Stephen Clarke [email protected] but the first by Stephen Clarke [email protected] (and only the former seems associated with your GitHub account?). Otherwise looks good now thanks.

@jrtc27 jrtc27 merged commit 9e20010 into CTSRD-CHERI:dev May 16, 2024
5 of 6 checks passed
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

Successfully merging this pull request may close these issues.

3 participants