- User Since
- Jan 14 2020, 12:20 AM (46 w, 7 h)
Wed, Nov 18
Updated lit tests based on latest source changes.
Tue, Nov 17
Fixed some of the review comments by Matt.
Mon, Nov 16
We had not arrived at any general consensus on which approach to stick to. Following were the sugguested proposals:
Fixed warnings by clang-tidy.
Sun, Nov 15
This revision was created just for tracking the progress of the work. Now that the code is ready for actual review, I created a new official revision - https://reviews.llvm.org/D91516, and hence closing this revision.
Add lit test-cases.
Sat, Nov 14
Retain original name for new cloned functions.
Make sure to run this pass as first AMDGPU IR pass.
Fixed minor bug in the updated code in the last commit.
Updated introductory comment.
Handle higher dimensional arrays.
Thu, Nov 12
Successfully tested an HIP application which defines different shared
variables of different types within different device functions, and there
exist call graph path from kernel to all these device functions.
Succesfully tested an HIP application which has more than one LDS globals.
Wed, Nov 11
Successfully tested simple sum-reduce HIP application.
Tue, Nov 10
Support for replacing constant expressions [in progress].
Mon, Nov 9
First cut of clode clean-up
Fixes previous incorrect commit.
First bug fix.
Support for indirect LDS globals.
Thu, Nov 5
Code refactoring as per the new logic of processing.
Wed, Nov 4
Partial support for indirect LDS globals.
Oct 30 2020
Completed the handling of direct LDS globals within kernel
Oct 27 2020
Fix bug and compute offset for each LDS
Oct 23 2020
Collect all associated LDS globals associated with a kernel.
Oct 22 2020
Implemented few more necessary data structures
Oct 21 2020
Aug 18 2020
The fix is already taken care in the patch https://reviews.llvm.org/D85772, hence, I am closing this patch.
Aug 13 2020
Looks like this patch and the patch https://reviews.llvm.org/D85772 fix the same issue. Me and Austin discussed about it, and decided to go ahead with https://reviews.llvm.org/D85772 instead of this patch. So, I will close this patch once https://reviews.llvm.org/D85772 is merged.
Add a comment
Fixed further review comments by Matt
Added LLVM lit test
Aug 11 2020
Jul 30 2020
I tested the SHOC benchmark and rocSPARSE testsuite for this new heuristic as suggested by Stas. The numbers are almost same as in case of previous heuristic (NumBytes <= 32).
Implemented new heuristic as suggested by Stas.
Jul 29 2020
Jul 23 2020
Fixed second review comment by Matt (though he said it is not mandatory)
Jul 22 2020
Take care of review comments by Matt.
IMHO, it is a good idea to add a generic utility support to get a virtual live-in register for a given physical register. Otherwise, each target land-up implementing it as I had done it for AMDGPU target sometime back. Thanks for cleaning it up. Except for my above minor comment, it is LGTM from my side unless other reviewers have any additional comments.
Jul 17 2020
This review request is no longer required. I am abandoning it.
And, I am closing this phabricator review.
And, further I reverted my original commmit (cc9d69385659be32178506a38b4f2e112ed01ad4) which had introduced faulty heuristic since I am not finding any quick solution here. Now, none of the issues should be blocked because of this faulty patch. I will now start from scratch again to arrive at the heauristic which hopefully purley based on number of custered-bytes without mixing-up with number of clustered instructions.
Jul 16 2020
I agree with you on your comments (and also with what @rampitec had made eariler).
Jul 13 2020
PING to give LGTM if the patch looks fine.
Rebased to upstream master.
Have taken care of review comment by Jay.
Jul 11 2020
Jun 23 2020
This patch was earlier got reviewed, accepted, and committed via https://reviews.llvm.org/D81085. But, I had to revert it because of the reasons updated in https://reviews.llvm.org/D81085. Now, those blocking issues are closed via https://reviews.llvm.org/D81649. But, meanwhile, few testcases which are updated in this patch, got changed, and conflicted. Hence, I again had to fix those test cases, and had to open this new revisoin.
Taken care of review comments by Jay.
Jun 11 2020
Jun 10 2020
Ok, this is interesting. Anyway, so, we need to make sure that SIInstrInfo::getMemOperandsWithOffsetWidth() will compute width for all cases. So, for the new case, that you have added, will you going to sumbit a patch which make sure that width is computed for this new case as well?
Jun 9 2020
This is surprizing, in my local repo the tests pass, but, it fails in pre-checkin build. I have locally built the patch from scratch with upstream rebased repo, and tested it as below:
Reverted in the commit - 7410571ce902c92087cb582b10710e17904d5b7a
Unit test failures in the latest Harbormaster build are spurious - llvm.amdgcn.image.nsa.ll actually passes in my local repo build, and other three failures are unsupported cases.
Jun 8 2020
Rebased to upstream master to make sure Harbormaster build is fine before commiting this patch.
Jun 7 2020
Rebase to latest upstream.
Jun 5 2020
Rebase to latest upstream
You already expressed that you have no objection with this patch in one of our internal email communications. As @foad expressed, it would be great if you also take a look at this patch, and officially give LGTM.
Have taken care of review comments by Jay.
Jun 4 2020
Fixed LLVM lit test regressions.