openrisc / mor1kx

mor1kx - an OpenRISC 1000 processor IP core
Other
490 stars 146 forks source link

Bug in mor1kx_cache_lru #119

Closed Harshitha172000 closed 3 years ago

Harshitha172000 commented 3 years ago

In mor1kx_cache_lru.v, lru_post is calculated as AND of rows

for (i = 0;i < NUMWAYS; i = i + 1) begin 
        lru_post[i] = &expand[i];      
end

Due to this, access and lru_post value are not the same.

This can be corrected by computing lru_post as the product of columns

 for (i = 0; i < NUMWAYS; i = i + 1) begin
        lru_post[i]=1;
        for (j=0;j<NUMWAYS;j=j+1)
          lru_post[i]= lru_post[i]*expand[j][i];
      end

Another bug detected, during formal verification. With the following code, the tool detects error.

for (i = 0; i<NUMWAYS; i = i + 1) begin    
    if (access[i]) begin            
     for (j = 0; j < NUMWAYS; j = j + 1) begin         
         if (i != j) begin                 
         expand[i][j] = 1'b0;              
         end           
    end            
   for (j = 0; j < NUMWAYS; j = j + 1) begin      
         if (i != j) begin           
       expand[j][i] = 1'b1;           
        end        
   end      
  end     
end

expand_error

Here, the third row of the expand matrix is of 3 bits instead of 4.

This can be corrected by precomputing the row exchange index.

 for (j=0;j<NUMWAYS;j=j+1) begin
       if (access[j])
         i=j;
 end
for (j = 0; j <NUMWAYS; j = j + 1) begin         
      if (i != j) begin                 
         expand[i][j] = 1'b0;              
       end           
end            
for (j = 0; j < NUMWAYS; j = j + 1) begin      
         if (i != j) begin           
              expand[j][i] = 1'b1;           
        end        
end  

no_expand_error (1)

stffrdhrn commented 3 years ago

As mentioned on the mail, the lru_post should not be the same as access.

So I don't think we should be changing that.

stffrdhrn commented 3 years ago

As per your second change, I mentioned its easier to exaplain by showing a patch, but this is kind of a rewrite so I agree its fine the way you presented it.

Do you know why splitting the for loop like you did fixes the problem?

Also could we combine the second for loop?

      //  3. Update the values with the access vector (if any) in the
      //     following way: If access[i] is set, the values in row i
      //     are set to 0. Similarly, the values in column i are set
      //     to 1.

      // First find the one hot bit in access
      for (j = 0; j < NUMWAYS; j = j + 1) begin
         if (access[j]) begin
             i = j;
         end
      end

      // Next update the expand matrix
      for (j = 0; j < NUMWAYS; j = j + 1) begin
         if (i != j) begin
              expand[i][j] = 1'b0;
              expand[j][i] = 1'b1;
         end
      end
Harshitha172000 commented 3 years ago

I misunderstood lru_post, no need for any modification.

Yeah, combining second for loop as you did will also work. It's optimized code.