-
Notifications
You must be signed in to change notification settings - Fork 35
[IR] LLVM cmpxchg Rework #976
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
base: development
Are you sure you want to change the base?
Conversation
Modify EventFactory.Llvm.newCompareExchange(...) Remove LlvmCmpXchg.setStructRegister()
dartagnan/src/main/java/com/dat3m/dartagnan/program/event/lang/llvm/LlvmCmpXchg.java
Outdated
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/program/event/EventFactory.java
Outdated
Show resolved
Hide resolved
| Preconditions.checkArgument(oldValueAndSuccessRegister.getType() instanceof AggregateType t | ||
| && t.getFields().size() == 2, | ||
| "Non-aggregate result register of LlvmCmpXchg."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add a check to be sure the part of the aggregate representing the comparison is bool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it is bv1 when parsed from LLVM IR. During compilation, VisitorBase creates a bool register.
| final var type = e.getResultRegister().getType() instanceof AggregateType t ? t : null; | ||
| verify(type != null && type.getFields().size() == 2, "Invalid result type of '%s'", e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not check directly in the verify that the type is an aggregate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this method needs type later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get it. When you define type you can generate a null (if the instance check does not hold) but then in the verify you do not allow type to be null. Why not simlply
verify(e.getResultRegister().getType() instanceof AggregateType);
verify(e.getResultRegister().getType().getFields().size() == 2, "Invalid result type of '%s'", e);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
He basically says that he wants to avoid doing the the cast to AggregateType part twice:
verify (type instance of AggregateType t && ...)
AggregateType t = (AggregateType) type; // "second" cast
// do something else with "t"
But I would totally prefer the version with the second cast instead of using nulls (null is always ugly).
| return List.of( | ||
| spinLoopHead, | ||
| newPthreadTryLock(oldValueRegister, successRegister, address, true), | ||
| EventFactory.newJump(successRegister, spinLoopEnd), | ||
| newPthreadTryLock(oldValueSuccessRegister, address), | ||
| EventFactory.newJump(expressions.makeExtract(oldValueSuccessRegister, 1), spinLoopEnd), | ||
| EventFactory.newGoto(spinLoopHead), | ||
| spinLoopEnd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed that our loop unrolling will unroll the spinloop but the dynamic side-effect detection will actually bound the loop to one iteration. I think it would be fair to generate a __VERIFIER_loopbound(1) event here to restrict the loop.
Well, at least if dynamic spin loop detection is enabled... but I think we might want to assume it is enabled?
This optimization could be applied to every CAS-loop that we generate ourselves (if there are more such cases).
Restructures the
LlvmCmpXchgevent from #332 into a single-result event using theAggregateTypefrom #493. This was the only event in our IR with two result registers.Also adds proper compilation of weak
LlvmCmpXchg. The intrinsics ofpthread_mutex_trylock,pthread_rwlock_tryrdlockandpthread_rwlock_trywrlocknow use strong events instead of weak ones. The API seems not to specify these to fail spuriously.