[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

This revision was automatically updated to reflect the committed changes. Closed by commit rG71ab6c98a0d1: [Matrix] Implement C-style explicit type conversions for matrix types. (authored by SaurabhJha, committed by fhahn). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

Re: [PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

Seems like the new build is passing. Can you please commit it on my behalf if it looks okay to you? Thanks a lot for your help in this patch. Saurabh On Fri, Apr 9, 2021 at 12:37 PM Saurabh Jha via Phabricator < revi...@reviews.llvm.org> wrote: > SaurabhJha added a comment. > > In

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

SaurabhJha added a comment. In D99037#2679131 , @fhahn wrote: > In D99037#2678848 , @SaurabhJha > wrote: > >> In D99037#2678779 , @fhahn wrote: >> >>> >> >> Will create a

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

SaurabhJha updated this revision to Diff 336395. SaurabhJha added a comment. Replace `matrices` with `matrixes` in comments and rewrite the comment about element types Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99037/new/

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

fhahn added a comment. In D99037#2678848 , @SaurabhJha wrote: > In D99037#2678779 , @fhahn wrote: > >> > > Will create a new patch to address your last comments Can you directly update this one? I'll commit it

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

SaurabhJha added a comment. In D99037#2678779 , @fhahn wrote: > Will create a new patch to address your last comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99037/new/

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

SaurabhJha added a comment. > LGTM, thanks for working on this! Thanks so much Florian. Can you please also commit this on my behalf as I don't have commit access? Cheers. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99037/new/

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

fhahn accepted this revision. fhahn added a comment. This revision is now accepted and ready to land. LGTM, thanks for working on this! Comment at: clang/include/clang/Sema/Sema.h:11715 + // CheckMatrixCast - Check type constraints for matrix casts. + // We allow casting

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

SaurabhJha added a comment. The windows build failure is solved by itself and its all passing now! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99037/new/ https://reviews.llvm.org/D99037 ___

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

SaurabhJha added inline comments. Comment at: clang/test/CodeGen/matrix-cast.c:82 + +void cast_unsigned_short_int_to_unsigned_int(unsigned_short_int_5x5 s, unsigned_int_5x5 i) { + // CHECK-LABEL: define{{.*}} void @cast_unsigned_short_int_to_unsigned_int(<25 x i16> %s, <25 x

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

SaurabhJha updated this revision to Diff 336154. SaurabhJha added a comment. Addressed latest round of comments. Also rebased with latest main as the windows build failed for some reason Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99037/new/

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

fhahn added a comment. Thanks for the latest update! This basically looks good to me, with a few final nits! Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8584 +def err_invalid_conversion_between_matrixes : Error< + "conversion between matrix type%diff{ $ and

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

SaurabhJha added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1219 + } else { +SrcElementTy = SrcTy; +DstElementTy = DstTy; fhahn wrote: > We should be able to assert here that both types are not matrix types, I > think? I did

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

SaurabhJha updated this revision to Diff 336022. SaurabhJha added a comment. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99037/new/ https://reviews.llvm.org/D99037 Files: clang/include/clang/AST/OperationKinds.def

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

fhahn added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1219 + } else { +SrcElementTy = SrcTy; +DstElementTy = DstTy; We should be able to assert here that both types are not matrix types, I think? Comment at:

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

SaurabhJha added a comment. Hopefully this will work. My IDE is a bit wonky and it will take hours to rebuild for me after rebase. So pushed here with the hope that this could be validated using web build. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

SaurabhJha added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1453 + CGF.CGM.getIntrinsic(llvm::Intrinsic::convert_to_fp16, + CGF.CGM.FloatTy), + Res); Not sure why this was changed. Perhaps

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

SaurabhJha updated this revision to Diff 335913. SaurabhJha added a comment. Rebased with latest main Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99037/new/ https://reviews.llvm.org/D99037 Files: clang/include/clang/AST/OperationKinds.def

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

SaurabhJha added a comment. > I think the issue might be that adding this additional cast-kind caused the > value to exceed the maximum supported by the `CastExprBitfields`; the > bitfield can only store 64 values, but after adding `MatrixCast`, > `CK_IntToOCLSampler` will have value `64`, so

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

fhahn added a comment. In D99037#2673367 , @SaurabhJha wrote: > Hey Florian and John, > > Thanks for your reviews so far. Just checked the build. Addressed all > previous comments and the build is looking good too except for one thing. For > open cl

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

SaurabhJha added a comment. Okay interesting I should have posted before. Seems like when I move `MatrixCast` to the bottom of OperationKinds.def, and do nothing else, the matrix-cast CodeGen fails with this error. It is somehow not able to assign correct cast type. + /tmp/build/bin/clang

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

SaurabhJha added a comment. Hey Florian and John, Thanks for your reviews so far. Just checked the build. Addressed all previous comments and the build is looking good too except for one thing. For open cl tests, it is failing with this curious `error: initializer element is not a

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

SaurabhJha updated this revision to Diff 335680. SaurabhJha added a comment. Fix the bug with int <-> float conversion by explicitly passing llvm types to EmitCastBetweenScalarTypes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99037/new/

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

SaurabhJha updated this revision to Diff 335677. SaurabhJha added a comment. I reverted the int <-> float conversion to previous code to make the tests pass. That way, we atleast have something working and we can go from there. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

SaurabhJha updated this revision to Diff 335643. SaurabhJha added a comment. Changes in latest revision: - Updated definition of areMatrixTypesOfTheSameDimension to reflect the comment - Refactored casting between types into EmitCastBetweenScalarTypes - Removed mentions of "non matrix" -

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

SaurabhJha added inline comments. Comment at: clang/include/clang/AST/OperationKinds.def:185 +/// CK_MatrixCast - A cast between matrix types of the same dimensions. +CAST_OPERATION(MatrixCast) + This line is causing me issues that I don't know how to solve. If

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

SaurabhJha added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1324 + +if (SrcElementTy->isFloatTy() || SrcElementTy->isDoubleTy()) { + QualType DstElementType = DstType->castAs()->getElementType(); fhahn wrote: > SaurabhJha wrote: >

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

fhahn added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1324 + +if (SrcElementTy->isFloatTy() || SrcElementTy->isDoubleTy()) { + QualType DstElementType = DstType->castAs()->getElementType(); SaurabhJha wrote: > fhahn wrote: > > I

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

SaurabhJha added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1324 + +if (SrcElementTy->isFloatTy() || SrcElementTy->isDoubleTy()) { + QualType DstElementType = DstType->castAs()->getElementType(); fhahn wrote: > I think we should

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

fhahn added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1324 + +if (SrcElementTy->isFloatTy() || SrcElementTy->isDoubleTy()) { + QualType DstElementType = DstType->castAs()->getElementType(); I think we should support all floating

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

fhahn added a comment. In D99037#2667484 , @SaurabhJha wrote: > Addressed most of the comments. I couldn't understand "..would also be good > to have C++ tests that test casting with matrix types where some of the > dimensions are template

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

SaurabhJha updated this revision to Diff 335216. SaurabhJha added a comment. Move back CK_MatrixCast definition to to just above CK_VectorSplat. The Matrix CodeGen is passing again but SemaOpenCL/sampler tests are failing again Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

SaurabhJha updated this revision to Diff 335182. SaurabhJha added a comment. Move the definition of MatrixCast to the bottom in OperationKinds.def Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99037/new/ https://reviews.llvm.org/D99037 Files:

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

SaurabhJha added a comment. In D99037#2668295 , @SaurabhJha wrote: > One other problem is somehow this test is failing > https://github.com/llvm/llvm-project/blob/main/clang/test/SemaOpenCL/sampler_t_overload.cl#L6 > with this error. > >

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

SaurabhJha added a comment. One other problem is somehow this test is failing https://github.com/llvm/llvm-project/blob/main/clang/test/SemaOpenCL/sampler_t_overload.cl#L6 with this error. /tmp/clang/test/SemaOpenCL/sampler_t_overload.cl:6:30: error: initializer element is not a

[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

SaurabhJha updated this revision to Diff 335084. SaurabhJha added a comment. Update one inline comment in SemaCast.cpp Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99037/new/ https://reviews.llvm.org/D99037 Files: