Register allocator correctness verifier#13229
Register allocator correctness verifier#13229danocmx wants to merge 112 commits intooracle:masterfrom
Conversation
|
Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application. When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated. If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public. |
oracle-contributor-agreement
Bot
added
the
OCA Required
|
Thank you for signing the OCA. |
oracle-contributor-agreement
Bot
added
OCA Verified
gergo-
left a comment
gergo-
left a comment
There was a problem hiding this comment.
I've done a general pass over about half of the code, adding some style and comment remarks. From my point of view, the most important thing for a thorough review would be to have documentation of the "big picture": What is the overall thing we are doing, and which classes cooperate in which way to achieve that goal.
The code seems clean overall, it's clear that you understand what you are doing, now we just need to understand it too :-)
| } | ||
|
|
||
| public boolean hasConflictedValue(ValueAllocationState valueAllocationState) { | ||
| for (var state : this.conflictedStates) { |
There was a problem hiding this comment.
It seems unfortunate that conflictedStates is a set but you need to do a linear search in it. Maybe instead of a set of states, it should be an RAValue -> ValueAllocationState map?
| * @return Newly copied state | ||
| */ | ||
| @Override | ||
| public abstract AllocationState clone(); |
There was a problem hiding this comment.
My understanding is that you should implement the Cloneable interface if you override Object.clone().
| * {@link UnknownAllocationState unknown} - our null state, nothing was stored yet - | ||
| * {@link ValueAllocationState value} - symbol that is stored at said location - | ||
| * {@link ConflictedAllocationState conflicted} - set of Values that are supposed to be at same | ||
| * location |
There was a problem hiding this comment.
Automated formatting destroyed your list, please use proper HTML list syntax in Javadoc comments.
| this.putWithoutRegCheck(entry.getKey(), UnknownAllocationState.INSTANCE); | ||
| } | ||
|
|
||
| var currentValue = this.internalMap.get(entry.getKey()); |
There was a problem hiding this comment.
This method would be a bit cleaner if you did this get once and then used the result everywhere, including currentValue == null instead of the containsKey call.
There was a problem hiding this comment.
I think it would make sense for this exception class and all of its subclasses to live in a separate subpackage.
| protected void checkStateValues(RAVInstruction.Op op) { | ||
| if (!op.hasCompleteState()) { | ||
| // Some values are null after allocation because of stack slot allocator | ||
| // because it is skipped when iteration (StackLockValue). |
There was a problem hiding this comment.
Where does this skipping of StackLockValues happen? Could you check more specifically for this situation?
| // TestCase: IntegerDivRemCanonicalizationTest | ||
| // instruction r10|QWORD = STACKLEA slot: stack:80|ILLEGAL[*] in B0 | ||
| // had vstack:0, which is not mentioned in first label or elsewhere | ||
| // so symbol vstack:0 won't be found |
There was a problem hiding this comment.
So basically you're not checking if the allocation of virtual stack slots to concrete stack slots is consistent in the same way that you check the consistency of the allocation of virtual registers to concrete registers?
| // not work, for example, RETURN with rax tends to contain the actual | ||
| // generated variable instead of rax symbol, or NEAR_FOREIGN_CALL | ||
| // keeps its own registers before and after allocation, but those | ||
| // can also contain different variable symbols. |
There was a problem hiding this comment.
It's not clear to me what "RETURN with rax tends to contain the actual generated variable instead of rax symbol" means. I'm probably missing some general context. You should write a package-info.java as an entry point that explains the big picture of how the verifier works and how the main classes work together. See vectorapi/package-info.java as an example.
| * In-case comparison of {@link ValueAllocationState} fails, it also might get resolved by this. | ||
| * </p> | ||
| */ | ||
| public interface ConflictResolver { |
There was a problem hiding this comment.
This interface only seems to have one implementation. Do you expect there to be more, or could the interface be removed?
| */ | ||
| public class RegAllocVerifierPhase extends RegisterAllocationPhase { | ||
| public static class Options { | ||
| @Option(help = "Verify that register allocation is indeed, correct", type = OptionType.Debug) public static final OptionKey<Boolean> EnableRAVerifier = new OptionKey<>(true); |
There was a problem hiding this comment.
Please put the @Option annotation on a separate line. The formatter will try to put it on the same line again, we usually wrap the whole block of option declarations in // @formatter:off/// @formatter:on. Alternatively, you can put an empty // comment at the end of the line to prevent merging.
| } | ||
|
|
||
| public static String getMessage(AllocatableValue orig, AllocatableValue curr, JavaKind kind) { | ||
| if (JavaKind.Object.equals(kind)) { |
There was a problem hiding this comment.
kind.isObject() would be a bit cleaner.
| /** | ||
| * Interface for state concrete location is in, stored in {@link AllocationStateMap}. | ||
| */ | ||
| public abstract class AllocationState implements Cloneable { |
There was a problem hiding this comment.
Looks like this could be a sealed class?
3 participants
Register allocation verifier, inspired by cranelift's regalloc checker. Works by saving symbols before allocation - variables, constants, registers and stack slots, then matches these to locations inserted by allocator(s) - registers and stack slots. Outputs of instructions then store these symbols to those locations in block's verification state. Spills, register moves and reloads move these around. Before any checking is done, initial allocation state (map of form location → symbol) is calculated for each block based on it's predecessors, when same location with different symbols meet, a conflict is created - can be resolved by writing to the location. Afterwards, checking is done for each block, we verify that symbols before allocation are stored at the locations inserted by allocator(s), taking into account reloads and spills that the allocator created.
Implemented as a phase (
RegAllocVerifierPhase) that wraps around both register and stack allocator, collects symbol information before allocation and matches this information to instructions after allocation to create verifier IR that it then uses to update state allocation maps and to verify operands.When a violation is detected by the verifier, a
RAVExceptionis thrown and if dumping is enabled a.rav.txtfile is created ingraal_dumpsdirectory with the exception details and verifier IR.Implemented as
jdk.graal.compiler.lir.alloc.verifierpackage, modifiesAllocationStageto insert the verification phase when enabled and also changes modifier to public forbuildmethod inLocationMarkerto access it in the verifier package. No other modification to existing was needed, variable locations stripped by the allocator from label/jump instructions are recovered by finding their first usage inFromUsageGlobalResolverclass to not mess with the existing register allocator code.Some functionality tested in
RegAllocVerifierTest, runs valid methods with the verifier, then injects faults and tests if the verifier can detect them. To be expanded.New compiler flags:
EnableRAVerifier- enables the verification for the compilationVerifyStackAllocator- verifies that stack allocator output together with register allocatorCollectReferences- usesLocationMarkerclass to collect GC reference sets before final code analysis stage to verify and invalidate old referencesOther functionality
alivelocation is not overwritten by the instruction astemporoutputJavaKindObject match