Skip to content

[Speculation] Switch to automatic buffering and normal CI flow#923

Open
murphe67 wants to merge 12 commits into
mainfrom
emmet/dev/new-speculation
Open

[Speculation] Switch to automatic buffering and normal CI flow#923
murphe67 wants to merge 12 commits into
mainfrom
emmet/dev/new-speculation

Conversation

@murphe67
Copy link
Copy Markdown
Collaborator

@murphe67 murphe67 commented May 18, 2026

Complete redo of speculator and save-commit RTL and communication between speculator and save-commit, as well as @shundroid's spec pre buffer units, to allow buffering using standard MILP approaches.

Further work: unit characterizations, fixing save-commit placement to avoid the single case of manual buffering, use of buffer characteristics to avoid pre buffer unit

@murphe67 murphe67 requested a review from Jiahui17 May 18, 2026 12:01
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This directory is for the old backend, right?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Technically? The system for including type dependencies is very strange and this the method we currently use for the existing VHDL multi dimensional outputs in the current backend.

We should probably change it, but for right now this follows the existing protocol for adding a new type def.

Comment on lines +68 to +71
- Merge the two speculator units into a single speculator unit
- Add an additional buffer on commit control inputs,
which is needed due to the conditionally-present join in the commit unit
being unrepresentable to the buffering algorithm.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why weren't they a single speculator unit to begin with?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Hoping to actually put them back into one unit later by doing some kind of buffer characterizations for how output latencies are a function of input latencies.

Currently I think you figured it out that when we pass a single speculator unit to the MILP it thinks the predicted output is generated after the real input arrives.

But actually the predicted output is generated as soon as the trigger arrives, and the control outputs are generated when the real input arrives. So we split it into which outputs are caused by which inputs.

}];
}

def SpecPreBufferOp1 : Handshake_Op<"spec_prebuffer1", [
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could it and Op2 have more descriptive names?

I initially thought that they were buffers, but they are actually two placeholders for letting the MILP to understand a speculator

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah this is definitely a good suggestion, will do 😁

let options = [];
}

def HandshakeSpecPostBuffer : DynamaticPass<"handshake-spec-post-buffer"> {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

DynamaticPass is deprecated: #284

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Should I call the name analysis stuff manually at the end of run on operation or do some pass manager stuff, do we have a agreed-on solution for this yet?

for (handshake::LSQOp lsqOp : funcOp.getOps<handshake::LSQOp>())
setLSQControlConstraints(lsqOp);

// The speculator has outputs which require buffers
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

could be a separate function like setLSQControlConstraints, to make the function body (setFPGA20Properties) look more balanced

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yeah agreed that it should be better encapsulated

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.

2 participants