-
Notifications
You must be signed in to change notification settings - Fork 26
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
Update VCLLVM (now Pallas) to LLVM 17, update to newest VerCors version, and convert more instructions to COL #1159
base: dev
Are you sure you want to change the base?
Conversation
Hey nice work, just a quick general comment while you work on this: I've realized that we're starting to do too much logic in the |
Yeah that makes sense I've so far not touched any of the transformation stuff so I don't quite know what goes in rewrite/, resolve/, etc. But indeed I already felt like some of the things I'm adding will be relevant more broadly. |
My current plan is to remove the changes from #1172 such that this can be merged without affecting any other parts of the code. I had some issues with the new Ubuntu 24.02 image (which I'm using to get LLVM-17) but the last few runs seem to have worked instead of being suddenly cancelled. |
… me and which aren't
in the transformations stages
…d clean up ClassToRef
df9ebc4
to
bede2fc
Compare
Marking this as ready for review since I basically just want to add some tests now before merging this |
def transSupportArrowsHelper( | ||
seen: Set[TClass[G]] | ||
): Seq[(TClass[G], TClass[G])] = { | ||
val t: TClass[G] = TClass(this.ref, typeArgs.map(v => TVar(v.ref))) | ||
// TODO: Does this break things if we have a ByValueClass with supers? |
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 would it? Do you want to disallow plain assignment for byref classes?
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.
The only thing I can come up with that's relevant is that in C, casting a MyStruct* to FirstMemberOfMyStruct* is well-defined. So in a way, a CStruct ast node does not have any supertypes, but the LangCToCol pass could compile it into a byvalue class, with the only supertype being equal to the first member of the struct. You'd have to account for that in assignment, though.
In addition, since you're targeting C, assigning different struct types makes no sense. So maybe struct types with supers should be disallowed? Then maybe byval classes should be separate from byref classes (not saying you have to do this, just thinking out loud)
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 agree it's probably best to not support ByValueClasses with super types for now
@@ -12,7 +12,7 @@ trait EndpointImpl[G] | |||
override def layout(implicit ctx: Ctx): Doc = | |||
Group(Text("endpoint") <+> ctx.name(this) <+> "=" <+> init) | |||
|
|||
def t: TClass[G] = TClass(cls, typeArgs) | |||
def t: TClass[G] = TByReferenceClass(cls, typeArgs) |
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 guess this might be a bit of a code smell: while this is the right choice for now as it reflects what was already the case, I did not have in mind to restrict endpoints of choreographies to be either byref or byval. I only expect fields and methods.
Maybe we can discuss in-office if it's possible or not to leave it open for both here? I thought the whole point of this PR was to leave it open in rewriting code whether a class is byref or byval, but I'm sure I'm missing a lot of (c++) context.
|
||
trait LLVMGlobalVariableImpl[G] extends LLVMGlobalVariableOps[G] { | ||
this: LLVMGlobalVariable[G] => | ||
// override def layout(implicit ctx: Ctx): Doc = ??? |
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.
Here and there there are some layout
methods not implemented. Maybe for those where it makes sense you can implement a real one, and for others maybe put in a placeholder value that makes a bit more sense than "???"
@@ -109,6 +109,9 @@ case class SourceName(name: String) extends NameStrategy { | |||
Some(SourceName.stringToName(name)) | |||
} | |||
|
|||
// Used to disambiguate whether to show a ByValueClass as a class or a struct | |||
case class TypeName(name: String) extends OriginContent |
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.
If you focus on the C frontend, why distinguish between ByValueClass that require a "class" prefix? In addition, there's no PVL syntax, right? So might aswell settle on that ByValueClasses are structs, and that ByRef is class...?
Spec.findClass(name, ctx) | ||
.getOrElse(throw NoSuchNameError("class", name, t)) | ||
) | ||
case t @ TByValueClass(ref, _) => |
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.
Just in case: if you make class/struct distinction, maybe you can change the "class" error message below to "struct" as well.
)) { dispatch(main) } | ||
if (mMap.isEmpty) { Let(b, v, m) } | ||
else { | ||
mMap.foreach(postM => |
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'm probably just slow, maybe we can have a look at this code later
@@ -131,7 +131,13 @@ case class EncodeForkJoin[Pre <: Generation]() extends Rewriter[Pre] { | |||
implicit val o: Origin = e.o | |||
cls.decls.collectFirst { case run: RunMethod[Pre] => run } match { | |||
case Some(_) => | |||
val obj = new Variable[Post](TClass(succ(cls), Seq())) | |||
val obj = | |||
cls match { |
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.
You had a "classType" helper for this, right...?
@@ -143,7 +143,7 @@ case class EncodeIntrinsicLock[Pre <: Generation]() extends Rewriter[Pre] { | |||
|
|||
override def dispatch(decl: Declaration[Pre]): Unit = | |||
decl match { | |||
case cls: Class[Pre] => | |||
case cls: ByReferenceClass[Pre] => |
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.
Unrelated: should intrinsicLockInvariant
for ByValueClass
throw an exception or something? Or maybe should we remove it from the Class trait?
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.
For now I assumed it would be fine to just make the lock invariant a fixed true
@@ -184,7 +184,11 @@ case class EncodeResourceValues[Pre <: Generation]() | |||
case ResourcePattern.HeapVariableLocation(_) => Nil | |||
case ResourcePattern.FieldLocation(f) => | |||
nonGeneric(fieldOwner(f)) | |||
Seq(TClass(succ(fieldOwner(f)), Seq())) | |||
Seq(fieldOwner(f) match { | |||
case cls: ByReferenceClass[Pre] => |
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.
Helper?
@@ -196,8 +200,12 @@ case class EncodeResourceValues[Pre <: Generation]() | |||
ref.args.map(_.t).map(dispatch) | |||
case ResourcePattern.InstancePredicateLocation(ref) => | |||
nonGeneric(predicateOwner(ref)) | |||
TClass[Post](succ(predicateOwner(ref)), Seq()) +: | |||
ref.args.map(_.t).map(dispatch) | |||
(predicateOwner(ref) match { |
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.
Helper?
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.
There you go! Good work!
val (patternsHere, body) = patterns.collect { | ||
// We only want to inline lets that are defined inside the quantifier | ||
letBindings.having(ScopedStack()) { dispatch(f.body) } | ||
localHeapVariables.scope { |
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.
Nitpick: maybe the concept of variables and heap variables can be unified behind one scope/trait? They both have reasonable get/set implementations, and heap variables further extend that with an address. Just a thought, might not be worth the effort either?
@@ -82,7 +82,7 @@ case class GenerateSingleOwnerPermissions[Pre <: Generation]( | |||
), | |||
) | |||
|
|||
case cls: Class[Pre] if enabled => | |||
case cls: ByReferenceClass[Pre] if enabled => |
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.
As an exercise you might consider integrating heap variables into this pass, though maybe you already do so somewhere else?
fieldTransitivePerm(e, f)(f.o) | ||
}) | ||
} | ||
case TClass(Ref(cls), _) => | ||
case t: TByReferenceClass[Pre] => |
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 do you prefer type matching over pattern matching here?
if !dereferencedHeapLocals.contains(System.identityHashCode(hl)) => | ||
v | ||
} | ||
VerificationError.withContext(CurrentRewriteProgramContext(program)) { |
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.
Can you not use program.rewrite here?
node match { | ||
// Same logic as CollectLocalDeclarations | ||
case Scope(vars, impl) => | ||
val (newVars, newImpl) = variables.collect { |
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.
Same here, scope.rewrite()?
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 see the difference, maybe it's even rewriteDefault in this case?
(a, b) match { | ||
case (None, _) | (_, None) => None | ||
case (Some(a), Some(b)) | ||
if a == b || rw.dispatch(a) == rw.dispatch(b) || |
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.
Comparing things in Post
is highly dubious! Might trigger comparison of some uninitialized refs. I know sycl used to do this every once in a while and it's a pain.
Is it maybe possible to make a more explicit ordering for a fragment of types? As this is for llvm, you only need to do this for say ints, structs, and the spec types (seq/map etc.). And for the spec types you can maybe just say to not let them interleave with llvm types maybe, to simplify things? Relying on the behaviour of another rewriter is fragile. It's also fine if your ordering is very limited and needs to be extended every once in a while. What about only using moreSpecific
, and maybe making moreSpecific
more complete for the identity cases?
} | ||
if (subMap.isEmpty) { value } | ||
else { | ||
// TODO: Support multiple guesses? |
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.
Only if there's an actual use case, I don't imagine any sane compiler does this (I'm probably wrong! :D)
e.o, | ||
) | ||
} | ||
// case Perm(a @ AmbiguousLocation(expr), perm) |
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.
Maybe you can just delete this? Otherwise please also comment why it is important to keep
@@ -88,7 +88,10 @@ case class LangTypesToCol[Pre <: Generation]() extends Rewriter[Pre] { | |||
case t @ PVLNamedType(_, typeArgs) => | |||
t.ref.get match { | |||
case spec: SpecTypeNameTarget[Pre] => specType(spec, typeArgs) | |||
case RefClass(decl) => TClass(succ(decl), typeArgs.map(dispatch)) | |||
case RefClass(decl: ByReferenceClass[Pre]) => | |||
TByReferenceClass(succ[Class[Post]](decl), typeArgs.map(dispatch)) |
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.
Probably you can put the [Post]
on TByRef, and then you don't need it on succ
case cls: ByReferenceClass[Pre] => | ||
globalDeclarations.succeed( | ||
cls, | ||
cls.rewrite(supports = |
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.
Here again maybe you can make a .rewrite
helper for ByRef/ByVal classes
Summary
This PR updates VCLLVM to LLVM 17 and updates it to work with the current VerCors. Additionally more instructions converted into COL, there is support for loops, pointers, structs, and more. This also includes changes to the C and PVL frontends for pointers and ByValueClass encoding. (i.e. structs)
Detailed list of changes
General
Pallas
C (changes superceding #1172 and #1227)
f
of struct valuea
becomesPerm(&a.f, write)
instead ofPerm(a.f, write)
a
becomesPerm(a, write)
(or ifa
is a pointer to a struct you'd havePerm(a, write) ** Perm(*a, write)
)unknownN
in the resulting viper file to help with debuggingPVL
*
and&
operators to PVL to work with pointer values (which could also already be used through thepointer<T>
ADT)--contract-import-file
which allows importing method contracts and types from a PVL file which will replace the function signature/contract of the LLVM function with the same name (this will likely be removed later once Robert's specification stuff gets merged in but is necessary now to test the LLVM frontend)To do: