[PATCH] D107963: [OpenCL] Fix as_type(vec3) invalid store creation

Tue Aug 17 06:12:27 PDT 2021

svenvh added inline comments.

================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4789
-
-    if (!CGF.CGM.getCodeGenOpts().PreserveVec3Type) {
-      Src = createCastsForTypeOfSameSize(Builder, CGF.CGM.getDataLayout(), Src,
----------------
Anastasia wrote:
> While I agree with this fix and it obviously looks incorrect, I wonder if the original intent was to condition the previous statement instead so that we avoid converting to size 4 at all? Although I have a feeling we are entering the behavior that is not documented anywhere. In the spec I can see this:
> 
> 
> ```
> When the operand and result type contain a different number of elements, the result shall be implementation-defined except if the operand is a 4-component vector and the result is a 3-component vector. In this case, the bits in the operand shall be returned directly without modification as the new type. 
> ```
> 
> but it seems to cover the inverse conversion?
Yeah I have a similar fix for the inverse case (which is further down in this function) in my local branch.

I did try to extend the guard to also cover the `ConvertVec3AndVec4` call, but that also led to invalid StoreInst creation.  Since I wasn't sure about the intent of the conditioning on `PreserveVec3Type` here, I didn't investigate further.

I was hoping @jaykang10 (who added this in D30810) might have some insight into why the guard was here in the first place.  But it has been over 4 years since that was committed, so there might not be a ready answer.  Either way, I'll hold off committing this for a few more days.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107963/new/

https://reviews.llvm.org/D107963