Page MenuHomePhabricator

SaurabhJha (Saurabh Jha)
User

Projects

User does not belong to any projects.

User Details

User Since
Feb 7 2021, 3:22 AM (10 w, 4 d)

Recent Activity

Sat, Apr 10

SaurabhJha updated subscribers of rG71ab6c98a0d1: [Matrix] Implement C-style explicit type conversions for matrix types..

Thanks Florian!

Sat, Apr 10, 4:11 AM

Fri, Apr 9

SaurabhJha updated subscribers of 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?

Fri, Apr 9, 8:28 AM · Restricted Project
SaurabhJha added a comment to D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

Will create a new patch to address your last comments

Can you directly update this one? I'll commit it after the update.

Fri, Apr 9, 4:37 AM · Restricted Project
SaurabhJha updated the diff for D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

Replace matrices with matrixes in comments and rewrite the comment about element types

Fri, Apr 9, 4:36 AM · Restricted Project
SaurabhJha added a comment to D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

Will create a new patch to address your last comments

Fri, Apr 9, 2:01 AM · Restricted Project
SaurabhJha added a comment to D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

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.

Fri, Apr 9, 1:59 AM · Restricted Project

Thu, Apr 8

SaurabhJha added a comment to D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

The windows build failure is solved by itself and its all passing now!

Thu, Apr 8, 11:37 AM · Restricted Project
SaurabhJha added inline comments to D99037: [Matrix] Implement C-style explicit type conversions for matrix types.
Thu, Apr 8, 9:50 AM · Restricted Project
SaurabhJha updated the diff for D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

Addressed latest round of comments.
Also rebased with latest main as the windows build failed for some reason

Thu, Apr 8, 9:44 AM · Restricted Project
SaurabhJha added inline comments to D99037: [Matrix] Implement C-style explicit type conversions for matrix types.
Thu, Apr 8, 12:44 AM · Restricted Project
SaurabhJha updated the diff for D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

Address comments

Thu, Apr 8, 12:41 AM · Restricted Project

Wed, Apr 7

SaurabhJha added a comment to D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

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.

Wed, Apr 7, 1:09 PM · Restricted Project
SaurabhJha added inline comments to D99037: [Matrix] Implement C-style explicit type conversions for matrix types.
Wed, Apr 7, 1:08 PM · Restricted Project
SaurabhJha updated the diff for D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

Rebased with latest main

Wed, Apr 7, 1:02 PM · Restricted Project
SaurabhJha added a comment to D100051: [clang] Move int <-> float scalar conversion to a separate function.

Alright, validating it now, then I'll push.

Wed, Apr 7, 12:28 PM · Restricted Project
SaurabhJha added a comment to D100051: [clang] Move int <-> float scalar conversion to a separate function.

I don't have commit rights so if it looks good to you, please commit on my behalf.

Wed, Apr 7, 11:51 AM · Restricted Project
SaurabhJha updated the diff for D100051: [clang] Move int <-> float scalar conversion to a separate function.

Rename the function to EmitScalarCast and directly returning from if branches now

Wed, Apr 7, 11:40 AM · Restricted Project
SaurabhJha added a comment to D100051: [clang] Move int <-> float scalar conversion to a separate function.

Hrmph... Phab ate my other comment, which was that the name EmitCastBetweenScalarTypes feels clunky. Does EmitScalarCast or EmitScalarScalarCast sound better and capture the meaning correctly?

Wed, Apr 7, 11:36 AM · Restricted Project
SaurabhJha added a comment to D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

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 assigning to the bitfield results in 0 being assigned. I *think* you have to bump the bitfield size to 7 or perhaps 8 (which may result in slightly better codegen). https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/Stmt.h#L521

This worked. Setting it to 7 made the tests pass.

Wed, Apr 7, 10:30 AM · Restricted Project
SaurabhJha added reviewers for D100051: [clang] Move int <-> float scalar conversion to a separate function: fhahn, rjmccall.
Wed, Apr 7, 10:29 AM · Restricted Project
SaurabhJha requested review of D100051: [clang] Move int <-> float scalar conversion to a separate function.
Wed, Apr 7, 10:28 AM · Restricted Project
SaurabhJha added a comment to D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

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.

Wed, Apr 7, 1:11 AM · Restricted Project
SaurabhJha added a comment to D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

Hey Florian and John,

Wed, Apr 7, 12:40 AM · Restricted Project

Tue, Apr 6

SaurabhJha updated the diff for D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

Fix the bug with int <-> float conversion by explicitly passing llvm types to EmitCastBetweenScalarTypes

Tue, Apr 6, 4:56 PM · Restricted Project
SaurabhJha updated the diff for D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

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.

Tue, Apr 6, 4:03 PM · Restricted Project
SaurabhJha updated the diff for D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

Changes in latest revision:

  • Updated definition of areMatrixTypesOfTheSameDimension to reflect the comment
  • Refactored casting between types into EmitCastBetweenScalarTypes
  • Removed mentions of "non matrix"
  • Replaced matrices with matrixes
  • Update error messages to "..is not allowed"
Tue, Apr 6, 1:14 PM · Restricted Project
SaurabhJha added inline comments to D99037: [Matrix] Implement C-style explicit type conversions for matrix types.
Tue, Apr 6, 11:52 AM · Restricted Project
SaurabhJha added inline comments to D99037: [Matrix] Implement C-style explicit type conversions for matrix types.
Tue, Apr 6, 11:48 AM · Restricted Project
SaurabhJha added inline comments to D99037: [Matrix] Implement C-style explicit type conversions for matrix types.
Tue, Apr 6, 11:19 AM · Restricted Project

Mon, Apr 5

SaurabhJha updated the diff for D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

Move back CK_MatrixCast definition to to just above CK_VectorSplat. The Matrix CodeGen is passing again but SemaOpenCL/sampler tests are failing again

Mon, Apr 5, 1:28 AM · Restricted Project

Sun, Apr 4

SaurabhJha updated the diff for D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

Move the definition of MatrixCast to the bottom in OperationKinds.def

Sun, Apr 4, 3:53 PM · Restricted Project
SaurabhJha added a comment to D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

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 compile-time constant
constant sampler_t glb_smp = 5;
                             ^
1 error generated.

Not sure what's going on but will continue to debug. Let me know if there's something simple that I am missing here.

Sun, Apr 4, 3:09 PM · Restricted Project
SaurabhJha added a comment to D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

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.

Sun, Apr 4, 12:28 PM · Restricted Project

Sat, Apr 3

SaurabhJha updated the diff for D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

Update one inline comment in SemaCast.cpp

Sat, Apr 3, 6:40 AM · Restricted Project
SaurabhJha retitled D99037: [Matrix] Implement C-style explicit type conversions for matrix types from [Matrix] Implement explicit type conversions for matrix types to [Matrix] Implement C-style explicit type conversions for matrix types.
Sat, Apr 3, 6:37 AM · Restricted Project
SaurabhJha updated the diff for D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

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 arguments...". When I tried this

Sat, Apr 3, 6:35 AM · Restricted Project

Fri, Apr 2

SaurabhJha added inline comments to D99037: [Matrix] Implement C-style explicit type conversions for matrix types.
Fri, Apr 2, 11:08 AM · Restricted Project
SaurabhJha updated the diff for D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

Update commit message to more accurately reflect the patch

Fri, Apr 2, 7:22 AM · Restricted Project
SaurabhJha added a comment to D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

Added some inline comments on where I have some doubts.

Fri, Apr 2, 6:31 AM · Restricted Project
SaurabhJha updated the diff for D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

Updating D99037: [Matrix] Implement explicit type conversions for matrix types
Removed unused variables and includes and fix codegen lit tests

Fri, Apr 2, 6:26 AM · Restricted Project
SaurabhJha added a comment to D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

I pushed some unwanted changes that I used for debugging..fixing/removing them now.

Fri, Apr 2, 6:20 AM · Restricted Project
SaurabhJha updated the diff for D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

Updating D99037: [Matrix] Implement explicit type conversions for matrix types

Fri, Apr 2, 6:13 AM · Restricted Project

Thu, Apr 1

SaurabhJha updated the diff for D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

Hi Florian and John,

Thu, Apr 1, 1:33 PM · Restricted Project
SaurabhJha added a comment to D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

Hey, elementary question about arcanist. I followed the steps here https://llvm.org/docs/Contributing.html#how-to-submit-a-patch and did

arc diff --edit --verbatim

on my current branch. Probably one mistake I did was to do a git reset HEAD~1 and create a new commit. Now its trying to create a new revision. Is that okay to create a new revision and abandoning this one?

Thu, Apr 1, 1:14 PM · Restricted Project
SaurabhJha abandoned D99765: [Matrix] Implement explicit type conversions for matrix types.
Thu, Apr 1, 1:08 PM · Restricted Project
SaurabhJha added a comment to D99765: [Matrix] Implement explicit type conversions for matrix types.

Sorry, this revision was created by mistake. I just wanted to update https://reviews.llvm.org/D99037

Thu, Apr 1, 1:08 PM · Restricted Project
SaurabhJha requested review of D99765: [Matrix] Implement explicit type conversions for matrix types.
Thu, Apr 1, 1:07 PM · Restricted Project

Tue, Mar 30

SaurabhJha added a comment to D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

Hey, I should mention that I am new to clang/llvm. My below thoughts could be wrong.

IIRC, making C-style casts work correctly in C++ will actually be easier if you make one of the specialized casts do it; I'd say go ahead and do it in static_cast.

Casts in C use basically a completely different code path.

Okay, then I think I will focus on static casts and C style casts for later. They use different functions, CheckStaticCast and CheckCStyleCast respectively and implementing static casts in matrix must involve change in CheckStaticCast and the function it calls.

Right, vector casts are their own unfortunate thing which we don't want to semantically emulate.

What code do you want to get out of this? Are there e.g. vectorized float->double conversions we can use, or is the operation basically doomed to break the matrix apart and put it back together again?

I think because matrices are vectors underneath, we should try vectorised conversions.

Tue, Mar 30, 11:30 AM · Restricted Project
SaurabhJha added a comment to D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

Thank you so much for your reviews. Apart from the rest of your comments, here are the two principle things I am going to do next:

  1. Replace the reinterpret_castwith static_cast. Do you think I should focus this revision to C-style casts and do static_casts in another patch?
  2. Create a new CK_MatrixCast and implement its handling.
Tue, Mar 30, 10:34 AM · Restricted Project
SaurabhJha added inline comments to D99037: [Matrix] Implement C-style explicit type conversions for matrix types.
Tue, Mar 30, 12:50 AM · Restricted Project

Wed, Mar 24

SaurabhJha added a comment to D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

I realise, as you pointed out before, that we need to do some changes in CodeGen too. I think its going to be more involved so before starting on that, it would be great if you can confirm whether I am on the right path. I will describe what I did, what happened, and what I inferred from that.

Wed, Mar 24, 2:59 PM · Restricted Project

Mar 22 2021

SaurabhJha added a comment to D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

I think you probably need to add new files for the tests, e.g. matrix-type-casts.cpp & matrix-type-casts.c and then just add tests for various valid cast combinations. Is that enough to get you started? You can look at some of the existing matrix codegen tests for inspiration for the run-line & CHECK lines.

Oh yes, missed the obvious thing I could have done, Cheers!

Mar 22 2021, 2:47 PM · Restricted Project
SaurabhJha added a comment to D99037: [Matrix] Implement C-style explicit type conversions for matrix types.

Thanks for the patch! I think this also needs changes in code-gen & code-gen tests.

Mar 22 2021, 2:25 PM · Restricted Project

Mar 21 2021

SaurabhJha requested review of D99037: [Matrix] Implement C-style explicit type conversions for matrix types.
Mar 21 2021, 9:43 AM · Restricted Project

Mar 7 2021

SaurabhJha added inline comments to D98075: [Matrix] Implement += and -= for MatrixType.
Mar 7 2021, 1:41 AM · Restricted Project
SaurabhJha updated the diff for D98075: [Matrix] Implement += and -= for MatrixType.

Fix mistake in Sema/matrix-type-operators.c's add test -= -> +=

Mar 7 2021, 1:38 AM · Restricted Project
SaurabhJha added a comment to D98075: [Matrix] Implement += and -= for MatrixType.

Thanks for the patch! Could you also extend the Sema matrix-type-operators tests with checks for += & -=?

It looks like the handling for multiply is slightly different and already works but is missing test coverage. It would be great if you could also extend the tests to check for *=.

Mar 7 2021, 1:07 AM · Restricted Project
SaurabhJha updated the diff for D98075: [Matrix] Implement += and -= for MatrixType.

Implement tests for *= for matrices code gen and add tests in Sema for compound assignments

Mar 7 2021, 1:03 AM · Restricted Project

Mar 5 2021

SaurabhJha updated the summary of D98075: [Matrix] Implement += and -= for MatrixType.
Mar 5 2021, 1:15 PM · Restricted Project
SaurabhJha requested review of D98075: [Matrix] Implement += and -= for MatrixType.
Mar 5 2021, 1:12 PM · Restricted Project