- User Since
- Apr 4 2014, 4:14 AM (375 w, 6 d)
Tue, Jun 15
Mon, Jun 14
LGTM apart from tidy comments on variable names.
LGTM. Please allow at least 24 hours for others to comment too before submitting. In the meanwhile PSDB and ePSDB runs will be useful.
Thu, Jun 10
Wed, Jun 9
We may change this logic further: return true only if TopCand.Reason is set (not NoCand anymore)
This seems to be better. Returning true with NoCand is misleading to me.
Looks good to me now.
Tue, Jun 8
Oops, too soon. Please add "amdgpu" to the switch.
LGTM as a short term w/a.
Can you add tests for GlobalAlias? It is not immediately clear if these are handled properly.
Mon, Jun 7
This probably will need rebase on top of D103655 which presumably shall simplify shouldLowerLDSToStruct() logic?
Thanks! It makes it now I believe. D103655 has @timestwo() which exploits partial conversion change. LGTM.
Changed initializer style.
Fri, Jun 4
I believe it was long needed.
I am not sure, if we can completely eliminate module lds. My understanding is that it is still required to handle within non-kernel function used LDS. But, instead of directly dealing with LDS, it should deal with pointers, hence the patch https://reviews.llvm.org/D103225.
Addressed review comments.
You need to replace HasGFX10_BEncoding with HasGFX10_AEncoding in the BVH and IMAGE_MSAA_LOAD_X. You also need to update llvm.amdgcn.image.msaa.load.x.ll test to include gfx1013.
Thu, Jun 3
I think from this we are just one step from removing module lds except for potentially indirect functions. Everything else can be moved into kernel lds structure.
I like the ISA changes, I'd say let's try it. If needed we could restrict it later with total LDS consumption calculation.
@hsmhsm please let a day before the submit in case if people have strong objections.
Tue, Jun 1
Same code added to selectDSGWSIntrinsic().
LGTM modulo Jay's comments.
Fix tidy warning.
Added verifier check.
Thu, May 27
Fix tidy warnings.
You probably need to wrap all prologue LDS stores into a block to execute it only from lane 0 and add a barrier after. @t-tye correct me if I am wrong.
Note the failure: LLVM.CodeGen/AMDGPU::llc-pipeline.ll
Generally LGTM. Please wait other reviewers too.
Any reason to force SA?
Thanks! I'd suggest to combine all fix-lds-alignmen*.ll tests into one. Just use different names for different blocks of variables so they are used only in one kernel.
JBTW, this patch directly reflects what's actually happen in HW. Even though these 3 instructions read 32 bit as a source internally they request to read 64 bit which in turn triggered alignment requirement. I assume this is really modeled as "read dword as an operand and have an implicit read of a superreg".