PSDB passed.

PSDB passed.

Fixed formatting.

In D103655#2823248, @rampitec wrote:

In D103655#2823247, @tra wrote:

Minimized reproducer: https://gist.github.com/Artem-B/44d8fa3f1bf0a3c992f4fe5bcf678c3f#file-lds-assert-ll

LLVM version I've tested with: 47f18af55fd59e813144cc76711806d57a160e50

$ bin/opt -amdgpu-lower-module-lds -disable-output LDS-assert.ll

Thanks, reproduced.

In D103655#2823247, @tra wrote:

Minimized reproducer: https://gist.github.com/Artem-B/44d8fa3f1bf0a3c992f4fe5bcf678c3f#file-lds-assert-ll

LLVM version I've tested with: 47f18af55fd59e813144cc76711806d57a160e50

$ bin/opt -amdgpu-lower-module-lds -disable-output LDS-assert.ll

In D103655#2823172, @tra wrote:

FYI. I've just got an assertion in the pass. I'll post a reduced reproducer when I have it.
Meanwhile here' the crash info:

F0616 14:20:09.488221 1150352 logging.cc:107] assert.h assertion failed at third_party/llvm/llvm-project/llvm/include/llvm/Support/Casting.h:269 in typename cast_retty<X, Y *>::ret_type llvm::cast(Y *) [X = llvm::PointerType, Y = llvm::Type]: isa<X>(Val) && "cast<Ty>() argument of incompatible type!"
*** Check failure stack trace: ***
    @     0x55555d4253df  absl::logging_internal::LogMessage::Die()
    @     0x55555d424e54  absl::logging_internal::LogMessage::SendToLog()
    @     0x55555d424b7f  absl::logging_internal::LogMessage::Flush()
    @     0x55555d425ae9  absl::logging_internal::LogMessageFatal::~LogMessageFatal()
    @     0x55555d4239c4  __assert_fail
    @     0x555558fddf77  llvm::GetElementPtrInst::Create()
    @     0x55555d1077d8  llvm::ConstantExpr::getAsInstruction()
    @     0x55555d201d42  llvm::convertConstantExprsToInstructions()
    @     0x55555d20125c  llvm::convertConstantExprsToInstructions()
    @     0x55555b47e1bd  llvm::AMDGPU::replaceConstantUsesInFunction()
    @     0x55555b2a40d5  (anonymous namespace)::AMDGPULowerModuleLDS::processUsedLDS()
    @     0x55555b2a31df  (anonymous namespace)::AMDGPULowerModuleLDS::runOnModule()
    @     0x55555d1bff34  llvm::legacy::PassManagerImpl::run()
    @     0x555558fae1b9  (anonymous namespace)::EmitAssemblyHelper::EmitAssemblyWithNewPassManager()
    @     0x555558fa9537  clang::EmitBackendOutput()
    @     0x555558fa66a5  clang::BackendConsumer::HandleTranslationUnit()
    @     0x555559c210b4  clang::ParseAST()
    @     0x5555599cb106  clang::FrontendAction::Execute()
    @     0x55555993fdcf  clang::CompilerInstance::ExecuteAction()
    @     0x555558bdbff3  clang::ExecuteCompilerInvocation()
    @     0x555558bcfd54  cc1_main()
    @     0x555558bcd6e7  ExecuteCC1Tool()
    @     0x555558bcd3fd  main
    @     0x7ffff7d29bbd  __libc_start_main
    @     0x555558bca0a9  _start

LGTM apart from tidy comments on variable names.

LGTM

LGTM. Please allow at least 24 hours for others to comment too before submitting. In the meanwhile PSDB and ePSDB runs will be useful.

In D103225#2809846, @hsmhsm wrote:

Insert @llvm.amdgcn.mbcnt.hi() for wave64 mode.

In D103225#2808671, @rampitec wrote:

In D103225#2808121, @hsmhsm wrote:

Implemented approach(2). Here we actually do not need builtin_amdgcn_mbcnt_hi(~0u, builtin_amdgcn_mbcnt_lo(~0u, 0u)). Irrespective of the wave64 or wave32, _builtin_amdgcn_mbcnt_lo(~0u, 0u) is enough. The reason is - we only want to identify lane 0. On the other hand, for wave64, if we wanted to identify any lane greater than 31, then we would need builtin_amdgcn_mbcnt_hi(~0u, builtin_amdgcn_mbcnt_lo(~0u, 0u)).

As far as I understand mbcnt_lo will return 0 for any thread >= 32, so you still need to use mbcnt_hi.

@b-sumner why do you suggest to nest the hi and lo calls? I think it shall be (builtin_amdgcn_mbcnt_lo(~0u, 0u) + builtin_amdgcn_mbcnt_hi(~0u, 0u)) == 0.

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.

In D103225#2808121, @hsmhsm wrote:

Implemented approach(2). Here we actually do not need builtin_amdgcn_mbcnt_hi(~0u, builtin_amdgcn_mbcnt_lo(~0u, 0u)). Irrespective of the wave64 or wave32, _builtin_amdgcn_mbcnt_lo(~0u, 0u) is enough. The reason is - we only want to identify lane 0. On the other hand, for wave64, if we wanted to identify any lane greater than 31, then we would need builtin_amdgcn_mbcnt_hi(~0u, builtin_amdgcn_mbcnt_lo(~0u, 0u)).

Looks good to me now.

Thanks!

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.

Rebased.

In D103800#2802465, @foad wrote:

Maybe this is stating the obvious, but you could add v6i32 and v6f32 types. @tpr did that before for v3 and v5 vectors, specifically for AMDGPU image ops. See D58901 (and a bunch of related commits you can find with git log --grep v5f32).

In D103225#2792240, @rampitec wrote:

In D103225#2785687, @rampitec wrote:

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.

But, I remember that we had decided to avoid barrier here, and instead just make sure that each thread within each wave execute the store instructions? In anycase, let me clarify it with @t-tye and @b-sumner.

I do not remember, but probably we can omit it since it is a singe store readonly memory. Anyway a confirmation from @t-tye would be nice.

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.

Rebased.

Changed initializer style.

I believe it was long needed.

In D103655#2797809, @rampitec wrote:

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.

We do have a problem with excessive use of lds in rocBLAS because of the module lds already, so technically we have a regression with it. There is a w/a but it is better be solved sooner rather than later.

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.

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.

@hsmhsm It depends on the convertConstantExprsToInstructions() you have implemented in the D103225. I would appreciate if you extract the helper into a separate review. Kudos for the helper BTW.

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.

Added comment.

Same code added to selectDSGWSIntrinsic().

In D103225#2785687, @rampitec wrote:

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.

But, I remember that we had decided to avoid barrier here, and instead just make sure that each thread within each wave execute the store instructions? In anycase, let me clarify it with @t-tye and @b-sumner.

LGTM modulo Jay's comments.

Fix tidy warning.

Added verifier check.

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.

In D103261#2785541, @arsenm wrote:

Do we actually see underaligned LDS variables in practice? What if the user is forcing a lower alignment to prioritize space utilization?

In D103261#2785495, @JonChesterfield wrote:

What's the use case for this? It will increase memory use if it increases the alignment of any variables

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".