Skip to content

Commit

Permalink
[aes/rtl] Replace long ternary expressions
Browse files Browse the repository at this point in the history 
This commit replaces long ternary expressions by a combination of
`if/else` and `unique case` statements.
  • Loading branch information
@vogelpi
vogelpi committed Oct 8, 2019
1 parent 3949b98 commit 68e3ea5
Showing 1 changed file with 87 additions and 17 deletions.
104 changes: 87 additions & 17 deletions hw/ip/aes/rtl/aes_control.sv
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,12 +185,35 @@ module aes_control #(
add_rk_sel_o = ADD_RK_INIT;

// Select key words for initial add_round_key
key_words_sel_o = dec_key_gen_q ? KEY_WORDS_ZERO :
(key_len_i == AES_128) ? KEY_WORDS_0123 :
(mode_i == AES_ENC && key_len_i == AES_192) ? KEY_WORDS_0123 :
(mode_i == AES_DEC && key_len_i == AES_192) ? KEY_WORDS_2345 :
(mode_i == AES_ENC && key_len_i == AES_256) ? KEY_WORDS_0123 :
(mode_i == AES_DEC && key_len_i == AES_256) ? KEY_WORDS_4567 : key_words_sel_e'(1'bX);
if (dec_key_gen_q) begin
key_words_sel_o = KEY_WORDS_ZERO;
end else begin
unique case (key_len_i)
AES_128: key_words_sel_o = KEY_WORDS_0123;

AES_192: begin
if (AES192Enable) begin
unique case (mode_i)
AES_ENC: key_words_sel_o = KEY_WORDS_0123;
AES_DEC: key_words_sel_o = KEY_WORDS_2345;
default: key_words_sel_o = key_words_sel_e'(1'bX);
endcase
end else begin
key_words_sel_o = key_words_sel_e'(1'bX);
end
end

AES_256: begin
unique case (mode_i)
AES_ENC: key_words_sel_o = KEY_WORDS_0123;
AES_DEC: key_words_sel_o = KEY_WORDS_4567;
default: key_words_sel_o = key_words_sel_e'(1'bX);
endcase
end

default: key_words_sel_o = key_words_sel_e'(1'bX);
endcase
end

// Make key expand advance - AES-256 has two round keys available right from beginning
if (key_len_i != AES_256 ) begin
Expand All @@ -210,12 +233,35 @@ module aes_control #(
state_we_o = ~dec_key_gen_q;

// Select key words for add_round_key
key_words_sel_o = dec_key_gen_q ? KEY_WORDS_ZERO :
(key_len_i == AES_128) ? KEY_WORDS_0123 :
(mode_i == AES_ENC && key_len_i == AES_192) ? KEY_WORDS_2345 :
(mode_i == AES_DEC && key_len_i == AES_192) ? KEY_WORDS_0123 :
(mode_i == AES_ENC && key_len_i == AES_256) ? KEY_WORDS_4567 :
(mode_i == AES_DEC && key_len_i == AES_256) ? KEY_WORDS_0123 : key_words_sel_e'(1'bX);
if (dec_key_gen_q) begin
key_words_sel_o = KEY_WORDS_ZERO;
end else begin
unique case (key_len_i)
AES_128: key_words_sel_o = KEY_WORDS_0123;

AES_192: begin
if (AES192Enable) begin
unique case (mode_i)
AES_ENC: key_words_sel_o = KEY_WORDS_2345;
AES_DEC: key_words_sel_o = KEY_WORDS_0123;
default: key_words_sel_o = key_words_sel_e'(1'bX);
endcase
end else begin
key_words_sel_o = key_words_sel_e'(1'bX);
end
end

AES_256: begin
unique case (mode_i)
AES_ENC: key_words_sel_o = KEY_WORDS_4567;
AES_DEC: key_words_sel_o = KEY_WORDS_0123;
default: key_words_sel_o = key_words_sel_e'(1'bX);
endcase
end

default: key_words_sel_o = key_words_sel_e'(1'bX);
endcase
end

// Make key expand advance
key_expand_step_o = 1'b1;
Expand Down Expand Up @@ -244,11 +290,35 @@ module aes_control #(
// Final round: do not update state anymore

// Select key words for add_round_key
key_words_sel_o = (key_len_i == AES_128) ? KEY_WORDS_0123 :
(mode_i == AES_ENC && key_len_i == AES_192) ? KEY_WORDS_2345 :
(mode_i == AES_DEC && key_len_i == AES_192) ? KEY_WORDS_0123 :
(mode_i == AES_ENC && key_len_i == AES_256) ? KEY_WORDS_4567 :
(mode_i == AES_DEC && key_len_i == AES_256) ? KEY_WORDS_0123 : key_words_sel_e'(1'bX);
if (dec_key_gen_q) begin
key_words_sel_o = KEY_WORDS_ZERO;
end else begin
unique case (key_len_i)
AES_128: key_words_sel_o = KEY_WORDS_0123;

AES_192: begin
if (AES192Enable) begin
unique case (mode_i)
AES_ENC: key_words_sel_o = KEY_WORDS_2345;
AES_DEC: key_words_sel_o = KEY_WORDS_0123;
default: key_words_sel_o = key_words_sel_e'(1'bX);
endcase
end else begin
key_words_sel_o = key_words_sel_e'(1'bX);
end
end

AES_256: begin
unique case (mode_i)
AES_ENC: key_words_sel_o = KEY_WORDS_4567;
AES_DEC: key_words_sel_o = KEY_WORDS_0123;
default: key_words_sel_o = key_words_sel_e'(1'bX);
endcase
end

default: key_words_sel_o = key_words_sel_e'(1'bX);
endcase
end

// Skip mix_columns
add_rk_sel_o = ADD_RK_FINAL;
Expand Down

3 comments on commit 68e3ea5

@cdgori
Copy link

@cdgori cdgori commented on 68e3ea5 Oct 16, 2019

Choose a reason for hiding this comment

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

I do prefer the ternary style here, it reads much better to me. (I believe you reverted to that after you updated the coding style guide?)

@vogelpi
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Chris,
thanks for your message. No I did not revert back to the ternary style even though I updated the style guide. There are three reasons why I didn't:

  1. Using a ternary does not allow to factor in the compile-time parameter AES192Enable. Factoring that it we can reduce complexity without fully relying on the synthesizer to detect that those cases won't be used.
  2. The ternary statements were very long, close to our 100 characters limit.
  3. I don't know if the ternary will still be suitable when we want to handle the default cases/don't cares differently. I think this might be easier with the combination of if/else + unique case

But I am open for discussion. We might want to reconsider once we know how to handle the default cases/don't cares. I suggest to see how the ternaries would look like then and then take the decision.

@cdgori
Copy link

@cdgori cdgori commented on 68e3ea5 Oct 17, 2019 via email

Choose a reason for hiding this comment

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

Please sign in to comment.