[SYCL][ESIMD] Lower global volatile stores to vstores #9088
Conversation
According to the VC team, all stores to volatile globals need to be vstores for correctness. Sometimes clang implicitly inserts stores, and if this happens, we need to lower to vstores. With that, we no longer need the commit() function, so remove it and update related doc. It never made it onto a compiler release, so we should be able to remove it with no deprecation. Signed-off-by: Sarnie, Nick <[email protected]>
Signed-off-by: Sarnie, Nick <[email protected]>
Signed-off-by: Sarnie, Nick <[email protected]>
There was a problem hiding this comment.
@sarnex - thank you for this fix.
I have couple of comments, of is minor, and another is major concern.
Signed-off-by: Sarnie, Nick <[email protected]>
| // of the simd object opertations, but in some cases clang can implicitly | ||
| // insert stores, such as after a write in inline assembly. To handle that | ||
| // case, lower any stores of genx_volatiles into vstores. | ||
| void lowerGlobalStores(Module &M, const SmallPtrSet<Type *, 4> &GVTS) { |
There was a problem hiding this comment.
You can pass SmallPtrSetImpl<Type *> to not carry 4.
There was a problem hiding this comment.
The GVTS variable is created and used elsewhere and already has type SmallPtrSet<Type *, 4>, and I don't think we are supposed to directly use Impl types, so I would prefer to leave this one as-is if that's okay.
There was a problem hiding this comment.
Impl types are a common pattern in LLVM to pass the ADT as parameter to the function.
Please refer to the Note section on the bottom of this chapter in LLVM Programmer's Manual.
There was a problem hiding this comment.
oh i didnt know of this use case, ill make a follow up pr to address this feedback, thx
Signed-off-by: Sarnie, Nick <[email protected]>
Follow-up from intel#9088 Signed-off-by: Sarnie, Nick <[email protected]>
…9120) Follow-up from #9088 Signed-off-by: Sarnie, Nick <[email protected]>
According to the VC team, all stores to volatile globals need to be vstores for correctness. Sometimes clang implicitly inserts stores, and if this happens, we need to lower to vstores.
With that, we no longer need the commit() function, so remove it and update related doc. It never made it onto a compiler release, so we should be able to remove it with no deprecation.