[AMDGPU][MC] Allow UC_VERSION_* constant reuse #96461
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-mc @llvm/pr-subscribers-backend-amdgpu Author: Carl Ritson (perlfu) ChangesIf more than one disassembler is created for a context then allow reuse of existing constants. Full diff: https://github.com/llvm/llvm-project/pull/96461.diff 1 Files Affected:
diff --git a/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp b/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
index 76a559c9443bd..d61e8ebb866df 100644
--- a/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
+++ b/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
@@ -2366,8 +2366,12 @@ const MCExpr *AMDGPUDisassembler::createConstantSymbolExpr(StringRef Id,
int64_t Val) {
MCContext &Ctx = getContext();
MCSymbol *Sym = Ctx.getOrCreateSymbol(Id);
- assert(!Sym->isVariable());
- Sym->setVariableValue(MCConstantExpr::create(Val, Ctx));
+ int64_t Res = ~Val;
+ assert(!Sym->isVariable() ||
+ (Sym->getVariableValue()->evaluateAsAbsolute(Res) && Res == Val));
+ (void)Res;
+ if (!Sym->isVariable())
+ Sym->setVariableValue(MCConstantExpr::create(Val, Ctx));
return MCSymbolRefExpr::create(Sym, Ctx);
}
|
kosarev
approved these changes
Jun 24, 2024
shiltian
reviewed
Jun 24, 2024
arsenm
reviewed
Jun 25, 2024
This might be difficult. |
Wouldn't this be just .set UC_VERSION_something something in an assembler file? |
Using the .set directive in an assembly file only tests the assembly override of the symbol. I've added an assembly override test and unit tests that cover the use of multiple diassemblers in the same context as well as UC_VERSION overrides. |
If more than one disassembler is created for a context then allow reuse of existing constants. Update assertion to validate constant contents.
- Change assertion to comment
arsenm
approved these changes
Jul 2, 2024
kosarev
reviewed
Jul 2, 2024
kosarev
reviewed
Jul 2, 2024
There was a problem hiding this comment.
We have some code downstream suffering from failures on the assertion in the disassembler. I wonder if we can provide a fix for that using the unit test from this patch and all the changes related to attempts to deal with changed symbol values removed (including in the unit test).
| const MCExpr *E = nullptr; | ||
| for (auto *VersionSym : UCVersionSymbols) { | ||
| int64_t Val; | ||
| if (!VersionSym->evaluateAsAbsolute(Val)) |
There was a problem hiding this comment.
Doing evaluateAsAbsolute() here doesn't prevent the symbols from being redefined later, so looks an unnecessary complication.
A proper solution would involve supporting constant symbols. Until then it seems whatever we do, it won't help us where any of the symbols get redefined.
There was a problem hiding this comment.
The point of the evaluation is to obtain the current value of the symbol for matching versions to it.
If the symbol is later redefined then the newer version should match.
| // Existing symbol may potentially have a different value to the one | ||
| // requested, which allows for user redefinition of symbols. | ||
| if (!Sym->isVariable()) { | ||
| Sym->setRedefinable(true); |
There was a problem hiding this comment.
Why is this call necessary?
There was a problem hiding this comment.
Allow redefinition at assembly stage.
| @@ -0,0 +1,41 @@ | |||
| // RUN: llvm-mc -triple=amdgcn -show-encoding -mcpu=gfx1010 %s | FileCheck --check-prefix=GFX10 %s | |||
|
|
|||
| // Test that UC_VERSION* symbols can be redefined. | |||
There was a problem hiding this comment.
This sounds like we want them be redefinable for some reason whereas it's actually quite the opposite.
As it is, this seem to simply test that symbols can be redefined, for which we probably already have some generic tests in MC/AsmParser?
There was a problem hiding this comment.
Just state this is nonsense and we just don't want to assert
There was a problem hiding this comment.
This test would pass with or without the code change.
If we think it is useful to check then I can add it back.
perlfu
force-pushed
the
amdgpu-dis-constant-reinit
branch
from
c5cea11 to
cbbc9b5
Compare
|
@kosarev - I am getting mixed messages here about what the desired solution is.
If we are not going to crash then, I believe we should give redefinition some defined/stable behaviour.
In which case this needs to use a different mechanism. I cannot reasonably put any more time in on this, so here is my final offer:
|
|
Ideally we would just raise an error if it's redefined, not assert |
I'm not against changing this to a |
kosarev
approved these changes
Jul 3, 2024
There was a problem hiding this comment.
LGTM, thanks! An error message or just removing the assert(!Sym->isVariable() || ... bit would be great.
If we are not going to crash then, I believe we should give redefinition some defined/stable behaviour.
Yes, so just letting it use the new (possibly incorrect) value without crashing should be fine for now as we don't have any better options. But us not being able to prevent redefinition doesn't mean we want the user to believe that there is any particular asm/disasm behaviour to be expected if they try to.
In which case this needs to use a different mechanism.
Yes. As soon as we have one.
- Restore redefinition unit tests to prove absence of crash.
I have made this output a warning to the MCContext. I have put back the redefinition unit test as proof that there is no crash. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Still LGTM. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/2193 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/39/builds/405 Here is the relevant piece of the build log for the reference: |
nico
added a commit
that referenced
this pull request
Jul 7, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/135/builds/313 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/79/builds/312 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/122/builds/197 Here is the relevant piece of the build log for the reference: |
|
@nico thank you for the quick fix |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If more than one disassembler is created for a context then allow reuse of existing constants.
Update assertion to validate constant contents.