[NFC][OpaquePtrs] Update SYCLLowerIR tests to use opaque pointers

[LU-JOHN]

Update SYCLLowerIR tests to use opaque pointers.

[LU-JOHN]

Current failures:

LLVM :: SYCLLowerIR/CompileTimePropertiesPass/sycl-properties-alignment-loadstore.ll
Test expects:
align 64
but output has:
"sycl-alignment"="64"
This failure existed before the changes in this PR.

LLVM :: SYCLLowerIR/ESIMD/global.ll
LLVM :: SYCLLowerIR/ESIMD/lower_vec_arg_fp.ll
LLVM :: SYCLLowerIR/ESIMD/lower_vec_arg_fp_metadata.ll
LLVM :: SYCLLowerIR/ESIMD/subroutine.ll
LLVM :: SYCLLowerIR/ESIMD/subroutine_extern.ll
LLVM :: SYCLLowerIR/hier_par_debug2.ll
These tests did not crash before this PR. These tests crash with the error message:

opt: /iusers/lujohn/src/SYCLOS/llvm/llvm/include/llvm/IR/Type.h:425: llvm::Type* llvm::Type::getNonOpaquePointerElementType() const: Assertion `NumContainedTys && "Attempting to get element type of opaque pointer"' failed.
PLEASE submit a bug report to https://github.com/intel/llvm/issues and include the crash backtrace.
Stack dump:
0. Program arguments: /localdisk2/lujohn/src/SYCLOS/llvm/build/bin/opt -passes=LowerESIMD,ESIMDLowerVecArg -S
#0 0x0000000001d19b21 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/localdisk2/lujohn/src/SYCLOS/llvm/build/bin/opt+0x1d19b21)
#1 0x0000000001d17544 SignalHandler(int) Signals.cpp:0:0
#2 0x00007f5bc2a0cdb0 __restore_rt (/lib64/libc.so.6+0x59db0)
#3 0x00007f5bc2a5942c __pthread_kill_implementation (/lib64/libc.so.6+0xa642c)
#4 0x00007f5bc2a0cd06 gsignal (/lib64/libc.so.6+0x59d06)
#5 0x00007f5bc29df7d3 abort (/lib64/libc.so.6+0x2c7d3)
#6 0x00007f5bc29df6fb _nl_load_domain.cold (/lib64/libc.so.6+0x2c6fb)
#7 0x00007f5bc2a05c86 (/lib64/libc.so.6+0x52c86)
#8 0x0000000001d53206 llvm::ESIMDLowerVecArgPass::run(llvm::Module&, llvm::AnalysisManagerllvm::Module&) (/localdisk2/lujohn/src/SYCLOS/llvm/build/bin/opt+0x1d53206)
#9 0x0000000001f63f1e llvm::detail::PassModel<llvm::Module, llvm::ESIMDLowerVecArgPass, llvm::PreservedAnalyses, llvm::AnalysisManagerllvm::Module>::run(llvm::Module&, llvm::AnalysisManagerllvm::Module&) (/localdisk2/lujohn/src/SYCLOS/llvm/build/bin/opt+0x1f63f1e)
#10 0x0000000001742b6c llvm::PassManager<llvm::Module, llvm::AnalysisManagerllvm::Module>::run(llvm::Module&, llvm::AnalysisManagerllvm::Module&) (/localdisk2/lujohn/src/SYCLOS/llvm/build/bin/opt+0x1742b6c)
#11 0x000000000077140d llvm::runPassPipeline(llvm::StringRef, llvm::Module&, llvm::TargetMachine*, llvm::TargetLibraryInfoImpl*, llvm::ToolOutputFile*, llvm::ToolOutputFile*, llvm::ToolOutputFile*, llvm::StringRef, llvm::ArrayRefllvm::PassPlugin, llvm::opt_tool::OutputKind, llvm::opt_tool::VerifierKind, bool, bool, bool, bool, bool, bool) (/localdisk2/lujohn/src/SYCLOS/llvm/build/bin/opt+0x77140d)
#12 0x00000000006d19bc main (/localdisk2/lujohn/src/SYCLOS/llvm/build/bin/opt+0x6d19bc)
#13 0x00007f5bc29f7e50 __libc_start_call_main (/lib64/libc.so.6+0x44e50)
#14 0x00007f5bc29f7efc __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x44efc)
#15 0x0000000000764f45 _start (/localdisk2/lujohn/src/SYCLOS/llvm/build/bin/opt+0x764f45)

[asudarsa]

Hi @LU-JOHN

The llorg community has removed all uses of getNonOpaquePointerElementType and deprecated it. There are two issues I see:

1. intel/llvm is a little bit behind, so we do not have some of the llorg patches that remove getNonOpaquePointerElementType
2. More relevant to your fails, ESIMD lowering still seems to use getNonOpaquePointerElementType. @sarnex, can you please comment on plans to fix this?

Thanks

[sarnex]

@asudarsa Hey, the only match I found was LowerESIMDVecArg. This pass actually is never run if opaque pointers are on, you can see that here. So I don't think there's any work to do.

We can just delete the pass whenever we pull down the change removing getNonOpaquePointerElementType.

[asudarsa]

@asudarsa Hey, the only match I found was LowerESIMDVecArg. This pass actually is never run if opaque pointers are on, you can see that here. So I don't think there's any work to do.

We can just delete the pass whenever we pull down the change removing getNonOpaquePointerElementType.

Ah. Interesting, Thanks so much @sarnex. I wonder why @LU-JOHN is seeing the call to this function in his stack trace above. One thing I noted, the checking condition in the code you pointed to, it has a call to supportsTypedPointers which is also deprecated. We might need a cleanup of sycl-post-link I suppose.
@LU-JOHN, can you please try to sync your branch with latest sycl tip and try to run these tests again?

Thanks

[bader]

We can just delete the pass whenever we pull down the change removing getNonOpaquePointerElementType.

Wouldn't you get a compilation error if method declaration is removed before all uses?

[sarnex]

Wouldn't you get a compilation error if method declaration is removed before all uses?

I meant as part of the pulldown. We can also remove it whenever we enable opaque pointers by default in this repo.

[bader]

Wouldn't you get a compilation error if method declaration is removed before all uses?

I meant as part of the pulldown. We can also remove it whenever we enable opaque pointers by default in this repo.

Let's remove the pass right after #9828 to make the process easier for the folks doing pulldown. Most likely they are not ESIMD experts and don't know if pass can be removed or not.

[sarnex]

@bader Sure just ping me when it gets merged.

[asudarsa]

pulldown

Thanks @bader. I agree. I think changes might be required across the board here. Here, we have noticed ESIMD lowering and sycl-post-link. Think there will be lot more. All components (SYCL related) might need to respond to this?

Thanks

[LU-JOHN]

Ah. Interesting, Thanks so much @sarnex. I wonder why @LU-JOHN is seeing the call to this function in his stack trace above.

I see the crash when invoking the opt tool, not sycl-post-link:

/SYCLOS/llvm/build/bin/opt < SYCLOS/llvm/llvm/test/SYCLLowerIR/ESIMD/global.ll -passes=LowerESIMD,ESIMDLowerVecArg -S

ESIMDLowerVecArg pass is initialized with opt.cpp:462

initializeESIMDLowerVecArgLegacyPassPass(Registry);

The crash occurs in line 714:

// The user has asked to use the new pass manager and provided a pipeline
// string. Hand off the rest of the functionality to the new code for that
// layer.
return runPassPipeline(argv[0], *M, TM.get(), &TLII, Out.get(),
                       ThinLinkOut.get(), RemarksFile.get(), Pipeline,
                       PluginList, OK, VK, PreserveAssemblyUseListOrder,
                       PreserveBitcodeUseListOrder, EmitSummaryIndex,
                       EmitModuleHash, EnableDebugify,
                       VerifyDebugInfoPreserve)

[LU-JOHN]

SYCLLowerIR/CompileTimePropertiesPass/sycl-properties-alignment-loadstore.ll does not fail before this PR. Mistaken conclusion is due to a stale build of opt in my build area. Edits/pulls to opt.cpp do not trigger a rebuild.

sycl-properties-alignment-loadstore.ll fails with the changes in this PR. Larger alignments expected by this testcase are not generated.

[sarnex]

@LU-JOHN Sorry, just to be super clear, would you like me to investigate the ESIMDLowerVecArg issue? Happy to do it, just not sure if needed.

[LU-JOHN]

@LU-JOHN Sorry, just to be super clear, would you like me to investigate the ESIMDLowerVecArg issue? Happy to do it, just not sure if needed.

@sarnex Yes, please help.

[LU-JOHN]

@broxigarchen Can you look at the changes in sycl-properties-alignment-loadstore.ll ? This PR changes:

%3 = bitcast i8 addrspace(4)* %2 to i32 addrspace(4)*
%4 = load i32, i32 addrspace(4)* %3, align 8
; CHECK: load {{.*}}, align 64

to:

%2 = load i32, ptr addrspace(4) %1, align 8
; CHECK: load {{.*}}, align 64

The CHECK no longer passes in this PR. Is the CHECK still valid?

[sarnex]

@LU-JOHN Sure, I just took a look and sorry, I should have been able to figure out the problem without actually debugging it lol.

The problem is we are running the pass directly, whereas when actually called from a real compiler run the pass won't get run if opaque pointers are disabled.

So I think the correct fix is to let all tests calling this pass directly to use typed pointers for now and then just delete the tests and the pass when we enable opaque pointers by default. There was already some talk of deleting the pass in the PR to enable opaqye pointers by default.

[sarnex]

PR to remove LowerESIMDVecArg #10738, still a draft but will publish once tests pass

[sarnex]

esimd test changes lgtm, i didn't review any tools files

[sarnex]

@bader ah thanks for requesting review explicitly from tools, i'll do that in the future, i didn't think of doing that

[broxigarchen]

@broxigarchen Can you look at the changes in sycl-properties-alignment-loadstore.ll ? This PR changes:

%3 = bitcast i8 addrspace(4)* %2 to i32 addrspace(4)*
%4 = load i32, i32 addrspace(4)* %3, align 8
; CHECK: load {{.*}}, align 64

to:

%2 = load i32, ptr addrspace(4) %1, align 8
; CHECK: load {{.*}}, align 64

The CHECK no longer passes in this PR. Is the CHECK still valid?

Hi. This PR should fix the issue
#10748

[MrSidims]

LGTM

[sarnex]

@intel/llvm-gatekeepers Can this one be merged? Here is a comment from @LU-JOHN:

It has two approvals, and it has a disabled test that exposed a bug in handling compileTimeProperty alignment with opaque pointers. This bug is fixed in PR 10748, but PR 10748 should enable the disabled test. (i.e. PR 10748 is waiting for 10687 to be merged).