Skip to content

Conversation

@elliotb-lowrisc
Copy link
Contributor

Attempt at neutral correction of what I hope are universal seen as spelling and capitalisation errors in some documentation and code comments.

Please flag any accidental changes to something that was already correctly spelt. My spell-checker is set to British, not American English, so there's a chance I accidentally "fixed" a spelling I didn't recognise as being the American variant.

@elliotb-lowrisc
Copy link
Contributor Author

Revert "practiced" -> "practised" change that turned out to be American spelling

@elliotb-lowrisc
Copy link
Contributor Author

Note that "implementors" -> "implementers" is not much of a fix, but as there were instances of both spellings I thought that bringing consistency was desirable. I went with "...ers" because it appears to be the more common spelling.

@elliotb-lowrisc
Copy link
Contributor Author

Removed another mistaken "correction" of American spelling


// Re-define gen_test_done() to override the base-class with an empty implementation.
// Then, our own overrided gen_program() can append new test_done code.
// Then, our own override gen_program() can append new test_done code.

Choose a reason for hiding this comment

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

I don't think "override" is correct here. "override" is a verb or noun, of which neither really make sense here, since we are using it as an adjective. "overrided" is not a word but could be considered semantically as either "overridden" or "overriding". I would suggest "overriding" is the correct choice here based on context.


// Create the ebreak insn which will cause us to enter debug mode, and run the
// special code in the debugrom.
// special code in the debug ROM.

Choose a reason for hiding this comment

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

This change is fine, but based on the context of 352f83f it might be more appropriate as "debug_rom" for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I think "debug ROM" is better for comments and documentation referring to the ROM itself and not a debug_rom class. This approach can already be found here:

virtual function void gen_debug_rom(int hart);
`uvm_info(`gfn, "Creating debug ROM", UVM_LOW)
debug_rom = ibex_hardware_triggers_debug_rom_gen::
type_id::create("debug_rom", , {"uvm_test_top", ".", `gfn});
debug_rom.cfg = cfg;
debug_rom.hart = hart;
debug_rom.gen_program();
instr_stream = {instr_stream, debug_rom.instr_stream};
endfunction

Is there a particular reason you prefer "debug_rom" here?

Choose a reason for hiding this comment

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

Just for consistency, but I hold no strong opinion either way. I think what you've said sounds fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I'll keep the current change, if you have no strong opinion

rtl/ibex_alu.sv Outdated
// of a, n = 8,16, or 32 for .b, .h, .w -variants. {a, b} denotes bit concatenation.
//
// Using barret reduction, one can show that
// Using Barret reduction, one can show that

Choose a reason for hiding this comment

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

Suggested change
// Using Barret reduction, one can show that
// Using Barrett reduction, one can show that


// Do not check for sequential increase after a branch, jump, exception, interrupt or debug
// request, all of which will set branch_req. Also do not check after reset or for dummys.
// request, all of which will set branch_req. Also do not check after reset or for dummies.

Choose a reason for hiding this comment

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

Nit: my personal preference would be to pluralize precisely as "dummy instructions", not "dummies".

Copy link

@AlexJones0 AlexJones0 left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this, @elliotb-lowrisc!

@elliotb-lowrisc elliotb-lowrisc added this pull request to the merge queue Jun 27, 2025
Merged via the queue into lowRISC:master with commit f0a57f0 Jun 27, 2025
11 checks passed
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