[PATCH] D126207: [AMDGPU][MC][GFX11] Support base+soffset+offset SMEM loads.
Dmitry Preobrazhensky via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue May 24 04:55:11 PDT 2022
dp added inline comments.
================
Comment at: llvm/lib/Target/AMDGPU/SMInstructions.td:1145
+ op, ps#"_SGPR_IMM", opName, (ins SReg_32:$soffset, smem_offset_mod:$offset)>;
+ def : MnemonicAlias<!cast<SM_Pseudo>(ps#"_COMMON").Mnemonic, opName>,
+ Requires<[isGFX11Plus]>;
----------------
kosarev wrote:
> dp wrote:
> > Is the _COMMON variant defined for aesthetic reasons? I believe the following code should work:
> >
> > def : MnemonicAlias<!cast<SM_Load_Pseudo>(ps#"_IMM").Mnemonic, opName>,
> >
> The aim here is to make the code reflect the fact that the pseudos share the same mnemonic (and I'm not sure casting to `SM_Load_Pseudo` instead of `SM_Pseudo` makes it obvious enough). This introduces a pseudo that is not used as such, but that seems a result of using pseudos to describe instruction properties -- a problem that we might want to address separately at some point.
This is a matter of taste, but for me the _COMMON definition does not make code clearer because the purpose of this definition is not immediately obvious. Maybe the name is a bit misleading and something like _MNEMO would be better?
Another (more important) problem is that the _COMMON definition if not for free - it contaminates compiler tables with unused entries.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126207/new/
https://reviews.llvm.org/D126207
More information about the llvm-commits
mailing list