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:
> 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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107963/new/
https://reviews.llvm.org/D107963
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
