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

LU-JOHN · 2023-08-03T21:44:22Z

Update SYCLLowerIR tests to use opaque pointers.

LU-JOHN · 2023-08-04T13:47:57Z

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 · 2023-08-04T14:00:35Z

Hi @LU-JOHN

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

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

Thanks

sarnex · 2023-08-04T14:03:31Z

@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 · 2023-08-04T14:13:13Z

@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 · 2023-08-04T16:12:35Z

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 · 2023-08-04T16:15:07Z

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 · 2023-08-04T16:20:22Z

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 · 2023-08-04T16:22:04Z

@bader Sure just ping me when it gets merged.

asudarsa · 2023-08-04T16:28:57Z

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 · 2023-08-04T22:02:35Z

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 · 2023-08-04T22:30:42Z

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 · 2023-08-07T13:58:36Z

@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 · 2023-08-07T15:01:14Z

@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 · 2023-08-07T15:09:00Z

@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 · 2023-08-07T16:44:16Z

@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.

Signed-off-by: Lu, John <[email protected]>

…s pass requires typed pointers. Signed-off-by: Lu, John <[email protected]>

Signed-off-by: Lu, John <[email protected]>

sarnex · 2023-08-08T15:43:10Z

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

… preserved after opaquification. Signed-off-by: Lu, John <[email protected]>

sarnex

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

sarnex · 2023-08-08T20:32:10Z

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

broxigarchen · 2023-08-09T01:34:05Z

@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 · 2023-08-17T19:38:24Z

@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).

LU-JOHN requested review from a team as code owners August 3, 2023 21:44

LU-JOHN marked this pull request as draft August 3, 2023 21:44

LU-JOHN temporarily deployed to aws August 3, 2023 21:59 — with GitHub Actions Inactive

LU-JOHN temporarily deployed to aws August 3, 2023 22:54 — with GitHub Actions Inactive

LU-JOHN added 2 commits August 7, 2023 13:05

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

509fe11

Signed-off-by: Lu, John <[email protected]>

Revert opaquify changes for tests that run ESIMDLowerVecArg pass. Thi…

887ab89

…s pass requires typed pointers. Signed-off-by: Lu, John <[email protected]>

LU-JOHN force-pushed the more-opaque branch from 5904fbd to 887ab89 Compare August 8, 2023 15:25

Opaquify removed GEP insn. Remove associated llvm.dbg.value call.

e9841bc

Signed-off-by: Lu, John <[email protected]>

Disable SYCL alignment properties testcase temporarily. Alignment not…

e4d1d24

… preserved after opaquification. Signed-off-by: Lu, John <[email protected]>

LU-JOHN marked this pull request as ready for review August 8, 2023 20:25

sarnex approved these changes Aug 8, 2023

View reviewed changes

bader requested a review from a team August 8, 2023 20:30

LU-JOHN mentioned this pull request Aug 9, 2023

[SYCL] fix opaque pointer issue in compileTimeProperty alignment #10748

Merged

MrSidims approved these changes Aug 14, 2023

View reviewed changes

bader merged commit cdd4dd3 into intel:sycl Aug 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

LU-JOHN commented Aug 3, 2023

LU-JOHN commented Aug 4, 2023

asudarsa commented Aug 4, 2023

sarnex commented Aug 4, 2023 •

edited

Loading

asudarsa commented Aug 4, 2023

bader commented Aug 4, 2023

sarnex commented Aug 4, 2023

bader commented Aug 4, 2023

sarnex commented Aug 4, 2023

asudarsa commented Aug 4, 2023

LU-JOHN commented Aug 4, 2023

LU-JOHN commented Aug 4, 2023 •

edited

Loading

sarnex commented Aug 7, 2023 •

edited

Loading

LU-JOHN commented Aug 7, 2023

LU-JOHN commented Aug 7, 2023

sarnex commented Aug 7, 2023 •

edited

Loading

sarnex commented Aug 8, 2023

sarnex left a comment •

edited

Loading

sarnex commented Aug 8, 2023

broxigarchen commented Aug 9, 2023

MrSidims left a comment

sarnex commented Aug 17, 2023 •

edited

Loading

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

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

Conversation

LU-JOHN commented Aug 3, 2023

LU-JOHN commented Aug 4, 2023

asudarsa commented Aug 4, 2023

sarnex commented Aug 4, 2023 • edited Loading

asudarsa commented Aug 4, 2023

bader commented Aug 4, 2023

sarnex commented Aug 4, 2023

bader commented Aug 4, 2023

sarnex commented Aug 4, 2023

asudarsa commented Aug 4, 2023

LU-JOHN commented Aug 4, 2023

LU-JOHN commented Aug 4, 2023 • edited Loading

sarnex commented Aug 7, 2023 • edited Loading

LU-JOHN commented Aug 7, 2023

LU-JOHN commented Aug 7, 2023

sarnex commented Aug 7, 2023 • edited Loading

sarnex commented Aug 8, 2023

sarnex left a comment • edited Loading

Choose a reason for hiding this comment

sarnex commented Aug 8, 2023

broxigarchen commented Aug 9, 2023

MrSidims left a comment

Choose a reason for hiding this comment

sarnex commented Aug 17, 2023 • edited Loading

sarnex commented Aug 4, 2023 •

edited

Loading

LU-JOHN commented Aug 4, 2023 •

edited

Loading

sarnex commented Aug 7, 2023 •

edited

Loading

sarnex commented Aug 7, 2023 •

edited

Loading

sarnex left a comment •

edited

Loading

sarnex commented Aug 17, 2023 •

edited

Loading