[LLVM] Fix CodeGenLLVM::LinkParameters

[kparzysz-quic]

• Generate valid LLVM IR.
• Set proper alignment on the constant variables.

[ANSHUMAN87]

• Generate valid LLVM IR.
• Set proper alignment on the constant variables.

@kparzysz-quic : could you please describe what was the issue? And possibly scenario to reproduce the issue. Thanks!

[kparzysz-quic]

The generated LLVM IR was not valid: for example it had bitcasts between pointer and array types.

[ANSHUMAN87]

The generated LLVM IR was not valid: for example it had bitcasts between pointer and array types.

Thanks for your response. Will it be possible for you to share one small snippets which will show LLVM IR before your change where the issue lies, and after your fix the resulting LLVM IR where the issue is resolved. TIA!

[kparzysz-quic]

If you use LLVM with assertions enabled, it will simply not allow you to create such a bitcast: attempt to do so will trigger an assertion.

[kparzysz-quic]

This is the assertion that I'm getting without this patch:

python3: /w/src/llvm-project/llvm/lib/IR/Type.cpp:689: static llvm::PointerType *llvm::PointerType::get(llvm::Type *, unsigned int): Assertion `isValidElementType(EltTy) && "Invalid type for pointer element!"' failed.

[kparzysz-quic]

The above assertion is because the program tried to get a pointer to void in LLVM. There is no such thing, hence the assertion. After you fix that, you hit another problem: the bitcast that was on line 221 was casting i8* %0 to [1 x i8*], which is not a legal cast, and so you get another assertion:

python3: /w/src/llvm-project/llvm/lib/IR/Instructions.cpp:2984: static llvm::CastInst *llvm::CastInst::Create(Instruction::CastOps, llvm::Value *, llvm::Type *, const llvm::Twine &, llvm::Instruction *): Assertion `castIsValid(op, S, Ty) && "Invalid cast!"' failed.

[ANSHUMAN87]

Thanks a lot @kparzysz-quic for clarification 👍

[areusch]

thanks for fixing this, @kparzysz-quic ! is it possible to add a unit test to demonstrate the incorrect usage?

[kparzysz-quic]

The test tests/python/unittest/test_link_params.py fails without this patch. How come it wasn't failing in the CI?

[kparzysz-quic]

I think I know what's going on: if you use LLVM that was compiled without assertions, it may not crash at the time of generating the LLVM IR. I guess that the generated LLVM IR ends up working correctly, at least in this case.

I use LLVM with built-in assertions, so for me it crashes. I don't know if there is a reliable way to make the current code fail without using assertions in LLVM.

[areusch]

@kparzysz-quic yeah i think you're right--we get LLVM from APT in the docker container, which i doubt has -DLLVM_ENABLE_ASSERTIONS as the website mentions it's slow. For that reason, I'm hesitant to suggest changing our LLVM install process to build one with assertions as it might significantly impact the CI speed. we could potentially add one version which contains assertions and run codegen unit tests against that. what are your thoughts?

[kparzysz-quic]

I've been using the entire clang toolchain built with assertions (the Release+Assertions build) in my daily work and I don't find it too slow. Frankly, I'm not sure if I could tell whether I'm running Release or Release+Assertions without some known references or comparing them side by side. I wouldn't be concerned at all about using such a build for CI. The slowness issue would definitely come up with a Debug build, which AFAIR is unoptimized.

[kparzysz-quic]

What else needs to be done for this PR?

[areusch]

ok, just wanted to understand those points. it seems like we should modify our LLVM install to build with assertions on in at least one version in one container. that doesn't have to block this PR, though. could you just clarify your answer to the one question about GEP and we can merge?

[areusch]

thanks @kparzysz-quic , the PR is now merged