-
Notifications
You must be signed in to change notification settings - Fork 281
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
[Pipeline] Add preliminary stall lowering #5467
Conversation
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.
Looks sane, but magic offset numbers get me really concerned. In my experience, they make code brittle.
size_t nStageInputArgs = parent.pipeline.getNumStageArguments(block); | ||
inputs = mod.getArguments().take_front(nStageInputArgs); | ||
llvm::SmallVector<Value> stageExtInputs; | ||
parent.getStageExtInputs(block, stageExtInputs); | ||
extInputs = | ||
mod.getArguments().slice(nStageInputArgs, stageExtInputs.size()); | ||
valid = mod.getArgument(mod.getNumArguments() - 3); | ||
valid = mod.getArgument(mod.getNumArguments() - (withStall ? 4 : 3)); |
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.
Please either break these magic numbers into well named constants or include inline comments.
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.
Or computing them dynamically by adding the variable length args together. Or by tracking them.
I've had trouble with assuming / recomputing offsets in the past (in the ESI port mapping code), so I refactored it to explicitly track them. Made the code much nicer.
valid = mod.getArgument(mod.getNumArguments() - 3); | ||
valid = mod.getArgument(mod.getNumArguments() - (withStall ? 4 : 3)); | ||
if (withStall) | ||
stall = mod.getArgument(mod.getNumArguments() - 3); | ||
clock = mod.getArgument(mod.getNumArguments() - 2); | ||
reset = mod.getArgument(mod.getNumArguments() - 1); |
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.
These assumed offsets make this lowering code brittle. Could you please break them out into well-named constants which are used everywhere in the lower in a subsequent "cleanup" PR?
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.
like 'resetOffsetFromEnd'?
And maybe add an assert that it's the expected value or type in places where you assemble an array?
Whenever I see magic numbers, I get concerned that future modifications will break this in a way not caught by a test.
I hate them just as much as you. Hoping to get #5473 in tree and then go back and fix these. |
eba4791
to
cfeb2e9
Compare
This commit adds preliminary stall lowering. Given the presence of a stall signal, every stage registers' clock enable value is now &'ed with the (pipeline) global stall signal.
310e0cd
to
08d3e64
Compare
This commit adds preliminary stall lowering.
Given the presence of a stall signal, every stage registers' clock enable value is now &'ed with the (pipeline) global stall signal.