Skip to content

Move MCSymbolRefExpr::VK_AMDGPU_ to AMDGPUMCExpr:: #130006

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MaskRay opened this issue Mar 6, 2025 · 3 comments
Open

Move MCSymbolRefExpr::VK_AMDGPU_ to AMDGPUMCExpr:: #130006

MaskRay opened this issue Mar 6, 2025 · 3 comments

Comments

@MaskRay
Copy link
Member

MaskRay commented Mar 6, 2025

MCSymbolRefExpr::VariantKind isn't ideal for encoding relocation operators because:

  • other expressions, like MCConstantExpr (e.g., PPC 4@l) and MCBinaryExpr (e.g., PPC (a+1)@l), also need it
  • semantics become unclear (e.g., folding expressions with @).
  • The generic interface MCSymbolRefExpr offers no target-specific extension point. Any target-specific logic will pollute the generic interface.

MCTargetExpr subclasses, as used by AArch64 and RISC-V, offer a cleaner approach.
(MIPS, while also uses MCTargetExpr, has significant tech debt.)
Ideally, limit MCTargetExpr to top-level use to encode one single relocation and avoid its inclusion as a subexpression.

AMDMCExpr::VariantKind is already present. Should just move more VK_AMDGPU_* there. @arsenm

Similar issue for VE: #130003


AArch64 is a good model where the VariantKind information is incoded as AArch64MCExpr (derived from MCTargetExpr)

// llvm/lib/Target/AArch64/AArch64MCInstLower.cpp
  Expr = AArch64MCExpr::create(Expr, RefKind, Ctx);
// llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
ImmVal = AArch64MCExpr::create(ImmVal, RefKind, getContext());
@llvmbot
Copy link
Member

llvmbot commented Mar 6, 2025

@llvm/issue-subscribers-backend-amdgpu

Author: Fangrui Song (MaskRay)

`MCSymbolRefExpr::VariantKind` isn't ideal for encoding relocation operators because:
  • other expressions, like MCConstantExpr (e.g., PPC 4@<!-- -->l) and MCBinaryExpr (e.g., PPC (a+1)@<!-- -->l), also need it
  • semantics become unclear (e.g., folding expressions with @).
  • The generic interface MCSymbolRefExpr offers no target-specific extension point. Any target-specific logic will pollute the generic interface.

MCTargetExpr subclasses, as used by AArch64 and RISC-V, offer a cleaner approach.
(MIPS, while also uses MCTargetExpr, has significant tech debt.)
Ideally, limit MCTargetExpr to top-level use to encode one single relocation and avoid its inclusion as a subexpression.

AMDMCExpr::VariantKind is already present. Should just move more VK_AMDGPU_* there. @arsenm

Similar issue for VE: #130003


AArch64 is a good model where the VariantKind information is incoded as AArch64MCExpr (derived from MCTargetExpr)

// llvm/lib/Target/AArch64/AArch64MCInstLower.cpp
  Expr = AArch64MCExpr::create(Expr, RefKind, Ctx);
// llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
ImmVal = AArch64MCExpr::create(ImmVal, RefKind, getContext());

@arsenm
Copy link
Contributor

arsenm commented Mar 6, 2025

I think we shouldn't need any of those relocations in the first place. They duplicate generic relocations, and the only difference is splitting the high and low halves of the final 64-bit address. I think these should be replaced with some kind of expression to extract the low and high bits, but I'm not sure how those work

@MaskRay
Copy link
Member Author

MaskRay commented Mar 9, 2025

There is no such a thing called a "generic relocation". All MCSymbolRefExpr::VariantKind should go away and MCSymbolRefExpr is not an appropriate place to place relocations (think of MCConstantExpr, MCBinaryExpr).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants