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

Wed Aug 18 01:27:09 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,
----------------
jaykang10 wrote:
> svenvh wrote:
> > jaykang10 wrote:
> > > 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.
> > No worries, thanks for replying!
> > 
> > > the goal was to avoid bitcast and keep load or store with vec3 type on IR level.
> > 
> > I think that is already achieved by the changes in CGExpr.cpp from your previous commit.  But here in CGExprScalar.cpp we are handling the case where we have to convert away to non-vec3 (because `NumElementsDst != 3`) and we do this conversion unconditionally already.  I don't see why we would not want to emit the bitcast because it is needed for correctness.
> > 
> > > It could be wrong for scalar type.
> > 
> > The problem that my patch fixes is not limited to scalar types: it also occurs for e.g. `float3` to `double2`.  Perhaps I should add that test case too?
> > 
> > > If you check the scalar type in the guard, it could be good to keep existing behavior for vector type.
> > 
> > My patch does not make a difference to any of the pre-existing tests in `preserve_vec3.cl`.  Do you have a specific case that is not covered by the test, but for which you want to preserve the behavior?
> > I think that is already achieved by the changes in CGExpr.cpp from your previous commit. But here in CGExprScalar.cpp we are handling the case where we have to convert away to non-vec3 (because NumElementsDst != 3) and we do this conversion unconditionally already. I don't see why we would not want to emit the bitcast because it is needed for correctness.
> I agree with you. I remember vaguely it was for a transformation pass in my previous project. For correctness, please feel free to remove the guard. 
> 
> >The problem that my patch fixes is not limited to scalar types: it also occurs for e.g. float3 to double2. Perhaps I should add that test case too?
> Yep, if you add more test cases, it will be great.
> 
> > My patch does not make a difference to any of the pre-existing tests in preserve_vec3.cl. Do you have a specific case that is not covered by the test, but for which you want to preserve the behavior?
> I can not remember correctly what my previous patch aimed. If someone raises issues with removing this guard later, I think we can discuss it again.
Thanks!  I will land the patch soon then.

> Yep, if you add more test cases, it will be great.

I'll add the float3 to double2 test case as part of my commit.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107963