parallaxsw / OpenSTA

GNU General Public License v3.0
52 stars 23 forks source link

BUG: CCSN Liberty parsing with `:` in `input_ccb` group #114

Closed akashlevy closed 3 weeks ago

akashlevy commented 3 weeks ago

Had a customer report an OpenSTA parsing issue with a commercial Liberty file. The problematic line looks like this (some PDK identifiers have been obfuscated here):

input_ccb(ECOMX2_D1_N_XXXXXXX_XXXX__XXX_1_4_XXXXX_XXXXX:a) {

Looks like a CCSN group. The customer is working on providing us an obfuscated test case, which I will attach.

I assume the fix is pretty straightforward: allow colons in groups? Let me know if you'd like me to try to make a PR for this @jjcherry56. Also let me know if there are any things I need to be aware of with allowing colons (maybe some other groups treat colons in a special way? Not sure)

akashlevy commented 3 weeks ago
library (ccs_input_ccb_colon) {
  delay_model : "table_lookup";
  simulation : false;
  capacitive_load_unit (1,pF);
  leakage_power_unit : "1pW";
  current_unit : "1A";
  pulling_resistance_unit : "1kohm";
  time_unit : "1ns";
  voltage_unit : "1v";
  library_features : "report_delay_calculation";
  input_threshold_pct_rise : 50;
  input_threshold_pct_fall : 50;
  output_threshold_pct_rise : 50;
  output_threshold_pct_fall : 50;
  slew_lower_threshold_pct_rise : 30;
  slew_lower_threshold_pct_fall : 30;
  slew_upper_threshold_pct_rise : 70;
  slew_upper_threshold_pct_fall : 70;
  slew_derate_from_library : 1.0;
  nom_process : 1.0;
  nom_temperature : 85.0;
  nom_voltage : 0.75;

  cell(ccs_input_ccb_colon) { 
    sensitization_master : sensitization_3pins ; 
    area : 0.1 ; 
    dont_touch : true ; 
    dont_use : true ; 

    pin(A) { 
      capacitance : 0.0001 ; 
      direction : input ; 
      driver_waveform_rise : "driver_waveform_default_rise" ; 
      driver_waveform_fall : "driver_waveform_default_fall" ; 
      fall_capacitance : 0.0001 ; 
      input_voltage : default ; 
      max_transition : 0.1 ; 
      related_ground_pin : VSS ; 
      related_power_pin : VDD ; 
      rise_capacitance : 0.0001 ; 

      input_ccb(ccs_input_ccb_colon:a) { 
        is_needed : true ; 
        is_inverting : true ; 
        miller_cap_fall : 0.0001 ; 
        miller_cap_rise : 1e-05 ; 
        stage_type : both ; 
      }
    }
  }
}
akashlevy commented 3 weeks ago
% read_liberty ccs_input_ccb_colon.lib
Error: ccs_input_ccb_colon.lib line 42, syntax error.

Line 42 is: input_ccb(ccs_input_ccb_colon:a) {

akashlevy commented 3 weeks ago

Created PR #115 that enables this to parse. Let me know if it looks ok.

akashlevy commented 3 weeks ago

Update: same user is reporting that a similar line is failing (below). Seems like we might need a slightly more robust solution.

active_input_ccb(GPDRFFQRLV_D1_N_S6P25TLTR_C54L04L06__nci_0_1_FR_RF:ck);
QuantamHD commented 3 weeks ago

I think given the lack of specification we should essentially treat () like string quotes and just take whatever is in there. I'm not sure if we're supposed to respect spaces though?

akashlevy commented 3 weeks ago

@jjcherry56 can decide how to treat the issue. My proposed solution is "conservative" in the sense that it minimizes the risk of breaking other things, while being less generalized...

akashlevy commented 3 weeks ago

I've updated my solution in #115 to allow colons in both complex attributes and groups

parallaxsw commented 3 weeks ago

fixed by a 1 character change in d889f037 issue114 liberty colons

akashlevy commented 3 weeks ago

Thanks :)

akashlevy commented 2 weeks ago

@jjcherry56 May I ask the motivation behind changing to the approach in 54d85a9? Did something break with the single-char change?

akashlevy commented 2 weeks ago

The general solution was rather nice. There were more issues that cropped up for the patching approach. For example, we encountered:

active_input_ccb(GPRLATQRLV_D1_N_S6P25TLTR_C54L04L06__nci_2_6_LFHHLFR_LRHHLRF:d, \
                                GPRLATQRLV_D1_N_S6P25TLTR_C54L04L06__nci_3_6_LFHHLFR_LRHHLRF:d);
propagating_ccb(ECOMX2_D1_N_S6P25TL_C54L04__nci_1_4_HFLLR_HRLLF:a, ECOMX2_D1_N_S6P25TL_C54L04__nco_0_0_FR_RF:y);
parallaxsw commented 2 weeks ago

Yes, unfortunately the simple change failed because there were libraries with attributes that crammed the colon against the attribute. variable_1: path_depth; I was trying to limit that allowing colons to the group names. I see another way to make the lexical solution work: 9f0997ef issue114 liberty colons round 3

akashlevy commented 2 weeks ago

3rd time's a charm, thanks a lot