[gen] Fix a relaxation merge problem.#1747
Conversation
fsestini
left a comment
fsestini
left a comment
There was a problem hiding this comment.
Left a few minor comments, but overall this looks fine to me.
The code being modified here -- responsible for merging edges and annotations -- is non-trivial. Therefore I'd be happier if we could introduce some regression tests. These do not need to be anything particularly complex. We could just have one test targeting the expected result of diyone7 -arch AArch64 L A L PosWR Fri -oneloc and a few other tests involving a mix of annotations and Store/Insert edges. These can be implemented as dune cram tests like we do, for example, in cat2config.
gen/common/edge.ml
Outdated
| List.rev es | ||
| |> List.find ( fun e -> not @@ is_insert_store e.edge ) in | ||
| List.fold_left (fun prev_annotation e -> | ||
| match is_insert_store e.edge, prev_annotation = e.a1 with |
There was a problem hiding this comment.
Let's not use polymorphic equality here.
I would advise against it in general, but especially in this particular case. Here we are comparing two terms of type atom for equality, but atom is an abstract type, so we don't actually know whether equality is well-defined for atoms. If there is the need to compare atoms for equality, we should extend the signature of AtomType to include a equal : atom -> atom -> bool function, and use that function for this comparison.
There was a problem hiding this comment.
Done.
I will use the existing function compare_atom instead of (=).
gen/common/edge.ml
Outdated
| |> Option.fold ~none:(update_annotation input) | ||
| (* Propagate result `f e` if changed *) | ||
| ~some:(fun e -> | ||
| Some(Option.value (update_annotation e) ~default:e)) in |
There was a problem hiding this comment.
Personal preference, but I'd find a plain old pattern match more readable, here.
There was a problem hiding this comment.
Will change to simple match.
Rework on the `resolve_edge` function in `edge.ml`. The new version will merge all annotations to immediate left and right hand side edges. This means the `merge_annotation` in `cycle.ml` become unnecessary.
ShaleXIONG
force-pushed
the
better-edge-merge
branch
from
8f01333 to
6be31f3
Compare
I add a syntax check in CI. It use a similar structure in #1582. Whichever pull get merged first, then I will rebase and manually merge the second. |
2 participants
Fix a problem in annotation merge, for example
diyone7 -arch AArch64 L A L PosWR Fri -onelocshould be an invalid input. It is due to theresolve_edgesedges function will simple ignore theAannotation inbetween twoLannotations, and the twoLwill be merged into oneL.We also remove an unnecessary second merge process in
cycle.ml.