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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
