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

Lower llvm intrinsics early #1363

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

bmanga
Copy link
Contributor

@bmanga bmanga commented May 24, 2024

The LowerAddrSpaceCast pass obfuscates all intrinsics calls, so the rest of the llvm pipeline doesn't recognize them.
Additionally, lowering early might mean that the subsituted code has a chance of getting optimized more aggressively.

The tests fallout is:

test/CPlusPlus/issue-357.cl

The old testCopyInstance2 is now optimized out (it was copying uninitialized memory). The test now initializes the data before the copy.

test/LLVMIntrinsics/descend_into_array.cl

The PR results in some instructions being re-ordered.

test/LLVMIntrinsics/memcpy_from_constant.cl

This seems to be a minor regression.
Instead of:

  %1 = getelementptr { [0 x float] }, ptr addrspace(1) %0, i32 0, i32 0, i32 0
  store float -2.000000e+00, ptr addrspace(1) %1, align 4, !dbg !40

the llvm ir before the producer is:

  %.elt = extractvalue [5 x float] [float -2.000000e+00, float -1.000000e+00, float 0.000000e+00, float 1.000000e+00, float 
  %1 = getelementptr { [0 x float] }, ptr addrspace(1) %0, i32 0, i32 0, i32 0
  store float %.elt, ptr addrspace(1) %1, align 4

This seems to be due to the HideConstantLoadsPass/UnhideConstantLoadsPass . Maybe it's too disruptive for the issue it was trying to fix? (#71)
I will open an issue for this regression.

This closes #1350 and #1355

@bmanga bmanga force-pushed the lower-llvm-intrinsics-early branch from 8e3a917 to 3e2bbd6 Compare May 28, 2024 16:06
lib/Compiler.cpp Outdated
Comment on lines 574 to 578

pm.addPass(clspv::ReplaceLLVMIntrinsicsPass());

if (clspv::Option::LanguageUsesGenericAddressSpace()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you put this addPass outside of the if statement?

If we really want it outside, maybe put it before the comment, and add comment before it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it outside because it helps in general, and I thought it's somewhat a big change for it to just change due to generic address space.
I moved it more to the top of the pipeline with a comment

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you provide a test showing how it helps in general, it's not clear to me looking at the modified tests. test/LLVMIntrinsics/memcpy_from_constant.cl even tends to say that it is not helping.

@bmanga bmanga force-pushed the lower-llvm-intrinsics-early branch from 3e2bbd6 to 918bbe6 Compare May 28, 2024 18:06
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.

Addrspace cast lowering pass creates invalid IR for intrinsic calls
3 participants