[TableGen] Sort matchables depending on predicates. #84483
Conversation
|
@llvm/pr-subscribers-mc Author: Alfie Richards (AlfieRichardsArm) ChangesThis change adds alphabetical sorting for matchables in Asm Matching. There are several cases of matchables that are unambiguous except for different features, for example the Arm Thumb ADC instruction which has multiple ambiguous aliases with different feature sets (see the test failures of #83436 for this failure). For Arm especially the order of matching determines the order of the diagnostics produced which can cause unreliable test failures. Full diff: https://github.com/llvm/llvm-project/pull/84483.diff 2 Files Affected:
diff --git a/llvm/test/MC/Mips/micromips32r6/valid.s b/llvm/test/MC/Mips/micromips32r6/valid.s
index b6af2b951c77c7..35eef58ae482d1 100644
--- a/llvm/test/MC/Mips/micromips32r6/valid.s
+++ b/llvm/test/MC/Mips/micromips32r6/valid.s
@@ -85,7 +85,7 @@
# CHECK-NEXT: # <MCInst #{{.*}} LH_MM
lhu $4, 8($2) # CHECK: lhu $4, 8($2) # encoding: [0x34,0x82,0x00,0x08]
# CHECK-NEXT: # <MCInst #{{.*}} LHu_MM
- lsa $2, $3, $4, 3 # CHECK: lsa $2, $3, $4, 3 # encoding: [0x00,0x43,0x24,0x0f]
+ lsa $2, $3, $4, 3 # CHECK: lsa $2, $3, $4, 3 # encoding: [0x00,0x83,0x14,0x0f]
lwpc $2,268 # CHECK: lwpc $2, 268 # encoding: [0x78,0x48,0x00,0x43]
lwm $16, $17, $ra, 8($sp) # CHECK: lwm16 $16, $17, $ra, 8($sp) # encoding: [0x45,0x22]
lwm16 $16, $17, $ra, 8($sp) # CHECK: lwm16 $16, $17, $ra, 8($sp) # encoding: [0x45,0x22]
@@ -194,7 +194,7 @@
msubf.d $f3, $f4, $f5 # CHECK: msubf.d $f3, $f4, $f5 # encoding: [0x54,0xa4,0x1b,0xf8]
mov.s $f6, $f7 # CHECK: mov.s $f6, $f7 # encoding: [0x54,0xc7,0x00,0x7b]
mov.d $f4, $f6 # CHECK: mov.d $f4, $f6 # encoding: [0x54,0x86,0x20,0x7b]
- # CHECK-NEXT: # <MCInst #{{[0-9]+}} FMOV_D64_MM
+ # CHECK-NEXT: # <MCInst #{{[0-9]+}} FMOV_D_MMR6
neg.s $f6, $f7 # CHECK: neg.s $f6, $f7 # encoding: [0x54,0xc7,0x0b,0x7b]
neg.d $f0, $f2 # CHECK: neg.d $f0, $f2 # encoding: [0x54,0x02,0x2b,0x7b]
# CHECK-NEXT: # <MCInst #{{[0-9]+}} FNEG_D64_MM
diff --git a/llvm/utils/TableGen/AsmMatcherEmitter.cpp b/llvm/utils/TableGen/AsmMatcherEmitter.cpp
index febd96086df27b..7c2f24c289557a 100644
--- a/llvm/utils/TableGen/AsmMatcherEmitter.cpp
+++ b/llvm/utils/TableGen/AsmMatcherEmitter.cpp
@@ -646,6 +646,14 @@ struct MatchableInfo {
if (RequiredFeatures.size() != RHS.RequiredFeatures.size())
return RequiredFeatures.size() > RHS.RequiredFeatures.size();
+ // Sort by the alphabetical naming of the required features.
+ for (unsigned i = 0, e = RequiredFeatures.size(); i != e; ++i) {
+ if (RequiredFeatures[i]->TheDef->getName() < RHS.RequiredFeatures[i]->TheDef->getName())
+ return true;
+ if (RHS.RequiredFeatures[i]->TheDef->getName() < RequiredFeatures[i]->TheDef->getName())
+ return false;
+ }
+
return false;
}
@@ -689,7 +697,19 @@ struct MatchableInfo {
HasGT = true;
}
- return HasLT == HasGT;
+ if (HasLT != HasGT)
+ return false;
+
+ // Check if can distringuish by the alphabetical ordering of features.
+ if (RequiredFeatures.size() != RHS.RequiredFeatures.size())
+ return false;
+ for (unsigned i = 0, e = RequiredFeatures.size(); i != e; ++i) {
+ if (RequiredFeatures[i]->TheDef->getName() < RHS.RequiredFeatures[i]->TheDef->getName()
+ || RHS.RequiredFeatures[i]->TheDef->getName() < RequiredFeatures[i]->TheDef->getName())
+ return false;
+ }
+
+ return true;
}
void dump() const;
|
|
@atanasyan I see you are a contributor to the MIPS backend. This changes the encodings of 2 test cases for MIPS as a different encoding is chosen. Is this okay? |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
I think adding (another) by-name ordering for matchables is not a good idea. Renaming things should not cause functional changes. I would rather welcome introducing proper means of communicating the intention/expectation of some matchables to be tried before others instead of complicating the situation further by employing ad-hoc workarounds. Did you try |
|
We need to figure out the real cause of non-deterministic behavior rather than masking it, so that we are not surprised in the future when it triggers again in a different place. Usually, such behavior is the consequence of using unstable keys (e.g., pointers) in hashtables. |
|
Yeah I agree the issue is probably actually the sort not being the same on the different builds. And youre right I think relying on the names is a bad idea.
I agree I think the real issue is why has this non-determinism creaped in that wasn't here previously. I will have a look at trying to compare these on Monday. It is quite frustrating to debug as it doesn't reliably repreoduce on my machine.
Ah I hadn't considered this. This would work well. I will abandon this patch and use that if the sort can't be figured out. |
In AMDGPU we had this kind of ambiguity sitting there for quite a while without noticing: #69256. Wouldn't be surprised if it's the same with other backends. |
|
I'm getting access to a windows machine so I will try work out whats going on. |
|
@s-barannikov here are the versions of the same commit (b1aef25) on windows vs Linux Ubuntu 22.02. I intend to do some investigation into where this difference originates. |
|
@s-barannikov I found the bug, its an embarassing one for me unfortunately, Im uploading a new patch to #83587 shortly. I will also be abandoning this PR as it is unnecessary for my change. I do think its a bit of an issue how many matchables aren't comparable right now even though they are functionally distinct and think there is an argument for something like this patch in the future, though as @kosarev points out, it should not be based off names. |
This change adds alphabetical sorting for matchables in Asm Matching.
There are several cases of matchables that are unambiguous except for different features, for example the Arm Thumb ADC instruction which has multiple ambiguous aliases with different feature sets (see the test failures of #83436 for this failure).
For Arm especially the order of matching determines the order of the diagnostics produced which can cause unreliable test failures.