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

[JinGu Kang via Phabricator via cfe-commits]

jaykang10 added inline comments.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4789
-
-    if (!CGF.CGM.getCodeGenOpts().PreserveVec3Type) {
-      Src = createCastsForTypeOfSameSize(Builder, CGF.CGM.getDataLayout(), Src,
----------------
svenvh wrote:
> 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.
I am sorry for late response. I has not been feeling well.

As far as I remember, the goal was to avoid bitcast and keep load or store with vec3 type on IR level. I guess I did not consider the conversion from vec3 type to scalar type and vice versa.

I guess this guard was to avoid the bitcast. It could be wrong for scalar type. If you check the scalar type in the guard, it could be good to keep existing behavior for vector type.

Additionally, you could also want to change below code for conversion from non-vec3 to vec3.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107963