Skip to content

Refactor EventFactory, Compilers and Parsers#1009

Open
xeren wants to merge 9 commits intodevelopmentfrom
refactorEventFactory
Open

Refactor EventFactory, Compilers and Parsers#1009
xeren wants to merge 9 commits intodevelopmentfrom
refactorEventFactory

Conversation

@xeren
Copy link
Copy Markdown
Collaborator

@xeren xeren commented Mar 13, 2026

Mostly cleans up some imports, but also

  • outlines some memory barrier generations in compilers.
  • outlines uses of ProgramBuilder.addChild in litmus parsers.
  • integrates FenceNameRepository into EventFactory

Modifications to EventFactory

Added

  • AArch64.newRMWLoadExclusive, AArch64.newRMWStoreExclusive
  • X86.newLoadFence, newStoreFence (still not implemented, but merges two places)
  • RISCV.newRMWLoadExclusive
  • Power.newRMWLoadExclusive

Modified

  • newRMWStoreExclusive, newRMWStoreExclusiveWithMo
  • AArch64.newDmbBarrier, etc.
  • AArch64.newBarrier(String,String) (moved from newFenceOpt)
  • RISCV.newRMWStoreConditional

@hernanponcedeleon
Copy link
Copy Markdown
Owner

@xeren can you please rebase and fix the conflicts?

…actory

# Conflicts:
#	dartagnan/src/main/java/com/dat3m/dartagnan/parsers/program/visitors/VisitorLitmusAArch64.java
#	dartagnan/src/main/java/com/dat3m/dartagnan/parsers/program/visitors/VisitorLitmusC.java
#	dartagnan/src/main/java/com/dat3m/dartagnan/parsers/program/visitors/VisitorLitmusPPC.java
#	dartagnan/src/main/java/com/dat3m/dartagnan/parsers/program/visitors/VisitorLitmusPTX.java
#	dartagnan/src/main/java/com/dat3m/dartagnan/parsers/program/visitors/VisitorLitmusRISCV.java
#	dartagnan/src/main/java/com/dat3m/dartagnan/parsers/program/visitors/VisitorLitmusVulkan.java
#	dartagnan/src/main/java/com/dat3m/dartagnan/parsers/program/visitors/VisitorLitmusX86.java
#	dartagnan/src/test/java/com/dat3m/dartagnan/others/miscellaneous/AnalysisTest.java
Copy link
Copy Markdown
Owner

@hernanponcedeleon hernanponcedeleon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you adding newRMWLoadExclusive for each arch if they all end up calling the general case? While I could see an argument in the factory for having the method for each arch, we do have other "common" events that use a single method, e.g. newRMWStoreExclusiveWithMo

AArch64.DSB.newISHLDBarrier() :
null;
Event fence = switch (e.getMo()) {
case Tag.C11.MO_RELEASE, Tag.C11.MO_SC -> AArch64.newDmbIshBarrier();
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are missing the mo.equals(C11.MO_ACQUIRE_RELEASE) case

@ThomasHaas
Copy link
Copy Markdown
Collaborator

Why are you adding newRMWLoadExclusive for each arch if they all end up calling the general case? While I could see an argument in the factory for having the method for each arch, we do have other "common" events that use a single method, e.g. newRMWStoreExclusiveWithMo

I agree with you but maybe from a different point of view. I think it becomes more and more unclear of whether a XYZ.newEvent factory method generates a core event or not. For parsers, you want to NOT generate core events to support compilation(*), but for the Compilation passes you need to ensure to generate only core events.
I think this is particularly true for the C11.newThreadFence events which should NOT be called by the parser (for the most part) and only during Compilation. And those can reasonably well be moved in the dedicated compilation pass.
For example, the LKMM compiler has some helper methods for the lowering that are not otherwise exposed to parsers/other callers.

(*) It is fine to generate core events from the parser, but not if you intend to support compilation to other targets on those events (at least in our current architecture).

@xeren
Copy link
Copy Markdown
Collaborator Author

xeren commented Mar 26, 2026

With the different newRMWLoadExclusive methods, I wanted to group all the usages in the program target-wise. It should have made them easier to be altered per-architecture. Ideally, Common, newRMWLoadExclusive, newLoadWithMo, newStoreWithMo and newFence are never used directly by parsers. As you noticed, my refactoring was incomplete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants