Branch Predictor Fixes & Improvements #148
Conversation
…r unconditional jumps
…ts are updated, GUI fixes
|
I have tested updated version with my simple test cases and all looks correct. I would wait for @jdupak for code style review and I hope we merge changes soon. |
| // depending on the setting | ||
|
|
||
| const machine::PredictorType predictor_type { config->get_bp_type() }; | ||
| const bool is_predictor_dynamic { predictor_type == machine::PredictorType::SMITH_1_BIT |
There was a problem hiding this comment.
nit: This logic would be better placed in the predictor module.
|
|
||
| number_of_bhr_bits = branch_predictor->get_number_of_bhr_bits(); | ||
| number_of_bht_bits = branch_predictor->get_number_of_bht_bits(); | ||
| initial_state = branch_predictor->get_initial_state(); | ||
| const machine::PredictorType predictor_type { branch_predictor->get_predictor_type() }; | ||
| const bool is_predictor_dynamic { predictor_type == machine::PredictorType::SMITH_1_BIT |
There was a problem hiding this comment.
This logic is used at two places -> it should be a function to make make sure it is consistent.
| zero_padding.fill('0', number_of_bhr_bits - binary_value.count()); | ||
| value_bhr->setText("0b" + zero_padding + binary_value); | ||
| for (uint16_t column_index = 0; column_index < bht->columnCount(); column_index++) { | ||
| QTableWidgetItem *item; |
There was a problem hiding this comment.
nit: Don't split initialization. This looks confusing.
| item = get_btb_cell_item(row_index, DOCK_BTB_COL_TYPE); | ||
| item->setData(Qt::DisplayRole, machine::branch_type_to_string(btb_entry.branch_type).toString()); | ||
| } else { | ||
| item = get_btb_cell_item(row_index, DOCK_BTB_COL_INSTR_ADDR); |
There was a problem hiding this comment.
I would really prefer not to reuse to variable. It actually confused me for a while. Actually, you dont need a variable at all. You could just call setData on the result of the get_... method.
| } | ||
|
|
||
| void DockPredictorInfo::set_update_widget_color(QString color_stylesheet) { | ||
| value_event_update_instruction->setStyleSheet(color_stylesheet); |
| number_of_bhr_bits = branch_predictor->get_number_of_bhr_bits(); | ||
| initial_state = branch_predictor->get_initial_state(); | ||
| const machine::PredictorType predictor_type { branch_predictor->get_predictor_type() }; | ||
| is_predictor_dynamic = predictor_type == machine::PredictorType::SMITH_1_BIT |
|
OK, I agree with incremental progress merging. The core changes are minimal and value of predictor is much bigger/usable for teaching in proposed version. I would like to add predictor disable action to all processor presets is some followup commit. |
|
I have made merge in parallel to your commit |
|
Thank you, the last commit should hopefully address all the things mentioned in the review. I pushed it without noticing the merge. |
Addresses some points from #143