[SYCL][FPGA] Changing the attribute max_private_copies to private_copes #994
Conversation
|
LGTM max_ here is really looking redundant. Please sign-off your commit. |
…ies. The IR coming out of this does not change. This allows the llvm-spirv translator to be affected. This should probably be fixed in the future (in the SPIRV spec) for consistency. Signed-off-by: Mohammad Fawaz <[email protected]>
| llvm::APSInt MCAInt = MCA->getValue()->EvaluateKnownConstInt(getContext()); | ||
| Out << '{' << MCA->getSpelling() << ':' << MCAInt << '}'; | ||
| Out << "{max_private_copies:" << MCAInt << '}'; |
There was a problem hiding this comment.
If this change is required, then:
| Out << "{max_private_copies:" << MCAInt << '}'; | |
| Out << "{private_copies:" << MCAInt << '}'; |
There was a problem hiding this comment.
Ah, I got it. In that way we don't want to change LLVM IR output. It's looking hacky, isn't it? :)
There was a problem hiding this comment.
Exactly.. As you mentioned, the "max" here is redundant, and is in fact misleading to customers. That's why we're getting rid of it. However, I'm keeping the IR the same for now because otherwise, we would have to change the llvm-spirv translator and the SPIRV spec to do things the right way. I should open a JIRA to track that effort, but this does not affect the customer flow.
There was a problem hiding this comment.
However, I'm keeping the IR the same for now because otherwise, we would have to change the llvm-spirv translator and the SPIRV spec to do things the right way.
I'd argue that no changes would have to be made to the SPIR-V specification, as it doesn't specify the LLVM IR layout. Would it be better to accompany this patch with a correspondent adjustment to the translator? I believe the fix would be trivial.
There was a problem hiding this comment.
@mohammadfawaz please address @AGindinson comment. I'm OK to go either way.
There was a problem hiding this comment.
@MrSidims @AGindinson I will address the translator changes asap!
…#994) According to https://llvm.org/docs/LangRef.html aliasing metadata must have following layout: List { MD Node Scope1, (other scopes) ... } Scope1 { MD Node Scope1, MD Node Domain1, (optional MD String) } ... (other scopes) Domain1 { MD Node Domain1, (optional MD String) } ... (other domains) and this pattern is actually being checked in LLVM's ScopedNoAliasAA.cpp. But in a harsh reality LLVM opt have bugs, which can result in this pattern violation. So lets be more tolerant to the input IR module. Signed-off-by: Dmitry Sidorov <[email protected]> Original commit: KhronosGroup/SPIRV-LLVM-Translator@2c61561
* sycl: (625 commits) [SYCL] Fix post-commit build failure (intel#3578) [SYCL] Add support for set(get)_specialization_constant (intel#3501) [SYCL] Do not allow template instantiation to create null attributes. (intel#3575) [SYCL][PI][L0] Close and submit batch immediately when Queue is empty. (intel#3552) [SYCL] Raise bit_cast to SYCL namespace (intel#3524) [ESIMD] Always preserve -vc-codegen option for ESIMD kernels (intel#3547) [SYCL] Fix warnings on clang-based build (intel#3570) Revert "Align tests with the codegen changes" Disable SPV_INTEL_memory_access_aliasing extension Fix build issues after applying translator patches [PassManager][PhaseOrdering] lower expects before running simplifyCFG Exclude spirv.hpp for clang-format Fix llvm-spirv crash when count of Fortran metadata variables is an array Fix transTypeComposite bug (intel#964) Fix incorrect translation of FPGA decoration on arrays (intel#983) Tolerate more inputs during alias.scope/noalias MD translation (intel#994) Update for LLVM iterator change Fix crash at translation of Entity of DebugImportedEntity (intel#951) Add HandleLLVMOptions to main CMakeLists.txt to propagate configuration flags. In particular, this enables Multi-threaded option in MSVC (instead of Multi-threaded DLL) Fix uninitialized variables warnings These warnings come into place when HandleLLVMOptions is imported ...
The IR coming out of this does not change. This allows the llvm-spirv
translator to not be affected. This should probably be fixed in the future
(in the SPIRV spec) for consistency.