Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed file naming issues when having multiple instances of an IP #58

Merged
merged 6 commits into from
Mar 18, 2020
Merged

Fixed file naming issues when having multiple instances of an IP #58

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5947ed8
Fixed file naming issues when having multiple instances of an IP
ElaHobby Mar 3, 2020
5e22d8b
Merge branch 'master' of https://github.com/Xilinx/RapidWright into Ela
ElaHobby Mar 4, 2020
9d9f4f5
Added support for 7 series RAMB and some Zynq dev
ElaHobby Mar 4, 2020
1bbb5f0
Added possibility to create partial.dcp for unfeasible placement
ElaHobby Mar 5, 2020
765cf5b
Limited Simulated Annealing runtime
ElaHobby Mar 6, 2020
7ad712d
Inserted pull request comments
ElaHobby Mar 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 45 additions & 4 deletions src/com/xilinx/rapidwright/design/blocks/PBlockGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public class PBlockGenerator {

/** X dimension over Y dimension */
public float ASPECT_RATIO = 0.125f;//1.5f;
public float OVERHEAD_RATIO = 1.5f;//1.25f;
public float OVERHEAD_RATIO = 4.5f;//1.25f;
Copy link
Member

Choose a reason for hiding this comment

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

@ElaHobby - I think we probably want to keep the OVERHEAD_RATIO default to 1.5. I'm sure you have found instances where it has needed to be increased, but we probably aren't ready to change it for everyone.

public int STARTING_X = -1; // Parameterized with command line argument when present.
public int STARTING_Y = -1; // Parameterized with command line argument when present.
public int PBLOCK_COUNT = 1;
Expand Down Expand Up @@ -940,16 +940,57 @@ public ArrayList<String> generatePBlockFromReport(String reportFileName, String
}
if(p.get(pIdx) == t) pIdx++;
}
sb.append("SLICE_X" + upperLeft.getInstanceX() + "Y" + (upperLeft.getInstanceY()-(numSLICERows-1)) +
":SLICE_X" + (upperLeft.getInstanceX()+(numSLICEColumns+numSLICEMColumns)-1) + "Y" + upperLeft.getInstanceY());

Copy link
Member

@clavin-xlnx clavin-xlnx Mar 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This update is clearly attempting to address the issue of the pblock colliding with the PS which is not a homogeneous--this is a problem. Since we have a number of situations similar to this, perhaps its best if we hold off on these point solutions and work together on a more holistic approach. I think in this current state, it may break solutions for other devices.

// Updated code
// Goal: code added for some 7 series devices, to avoid illegal placement due to clb-free portions in the device
int upperRX = upperLeft.getInstanceX()+(numSLICEColumns+numSLICEMColumns)-1;
int upperLX = upperLeft.getInstanceX();
int upperLY = upperLeft.getInstanceY();
boolean allowed_xc7z020 = dev.getName().contains("xc7z020") && !(( ((upperLX<68)&&(upperRX>67)) || ((upperLX>67)&&(upperLX<80)) || ((upperLX<80)&&(upperRX>79)) ) && (upperLY>49));
boolean allowed_xc7z030 = dev.getName().contains("xc7z030") && !(( ((upperLX<72)&&(upperRX>71)) || ((upperLX>71)&&(upperLX<84)) || ((upperLX<84)&&(upperRX>83)) ) && (upperLY>99));
boolean allowed_xc7z045 = dev.getName().contains("xc7z045") && !(( ((upperLX<124)&&(upperRX>123)) || ((upperLX>123)&&(upperLX<136)) || ((upperLX<136)&&(upperRX>135)) ) && (upperLY>249));
boolean no_xc7z030_xc7z020 = !dev.getName().contains("xc7z030") && !dev.getName().contains("xc7z020") && !dev.getName().contains("xc7z045");

// Try moving the pblock lower. Completly removing it is not optimal, as there is a risk that the pattern bellow the gap won't be used at all
if(!allowed_xc7z030 && dev.getName().contains("xc7z030")) {
upperLY = 95;
if((upperLY-(numSLICERows-1))>0) // Do I have enough rows? If not, placement unfeasible
allowed_xc7z030 = true;
}

if(!allowed_xc7z045 && dev.getName().contains("xc7z045")) {
upperLY = 245;
if((upperLY-(numSLICERows-1))>0) // Do I have enough rows? If not, placement unfeasible
allowed_xc7z045 = true;
}

if(!allowed_xc7z020 && dev.getName().contains("xc7z020")) {
upperLY = 45;
if((upperLY-(numSLICERows-1))>0) // Do I have enough rows? If not, placement unfeasible
allowed_xc7z020 = true;
}

if(allowed_xc7z020 || allowed_xc7z030 || no_xc7z030_xc7z020 || allowed_xc7z045)
sb.append("SLICE_X" + upperLX + "Y" + (upperLY-(numSLICERows-1)) + ":SLICE_X" + upperRX + "Y" + upperLY);
else
continue; // If pblock not feasible, jump over this pblock, look for other patterns
// Bellow, old code
// sb.append("SLICE_X" + upperLeft.getInstanceX() + "Y" + (upperLeft.getInstanceY()-(numSLICERows-1)) +
// ":SLICE_X" + (upperLeft.getInstanceX()+(numSLICEColumns+numSLICEMColumns)-1) + "Y" + upperLeft.getInstanceY());
// End of updated code





}
if(numBRAMColumns > 0){
int pIdx = 0;
for(int i=0; pIdx < p.size(); i++){
TileTypeEnum t = dev.getTile(row, col+i).getTileTypeEnum();
if(Utils.isBRAM(t)){
for(Site s : dev.getTile(row, col+i).getSites()){
if(s.getSiteTypeEnum() == SiteTypeEnum.RAMBFIFO36) upperLeft = s;
if((s.getSiteTypeEnum() == SiteTypeEnum.RAMBFIFO36)||(s.getSiteTypeEnum() == SiteTypeEnum.RAMBFIFO36E1)) upperLeft = s; // Update. Goal: support for 7 series
Copy link
Member

Choose a reason for hiding this comment

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

Thanks, this change looks good. I will follow up later with a better check for BRAMs.

}
break;
}
Expand Down
35 changes: 34 additions & 1 deletion tcl/rapidwright.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,12 @@ proc compile_block_dcp { dcpFile } {
puts "RAPIDWRIGHT_PATH=$rwpath"
puts "CLASSPATH=$cpath"
exec java com.xilinx.rapidwright.util.Unzip ${dcpFile} ${unzipDir}
read_xdc ${unzipDir}/${rootDcpFileName}_in_context.xdc
# Updated. Goal: avoid naming problems caused by the fact that the files copied into IP_CACHE have different names as the ones expected by RW. Error appears only in designs with multiple IPs with the same ID
# Line bellow removed and replaced by the following 2 lines
#read_xdc ${unzipDir}/${rootDcpFileName}_in_context.xdc
set file_name_xdc [glob -directory ${unzipDir} *_in_context.xdc]
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it should work. I'm ok with this change, you can delete the old line as it stays in the history of git.

read_xdc $file_name_xdc
# End of Updated
file delete -force ${unzipDir}
# END Work around

Expand Down Expand Up @@ -394,6 +399,12 @@ proc prep_for_block_stitcher {} {
puts "WILL RUN: $ip_run $name $ip $id $ip_cell"
}
} elseif { [needs_impl_run $cachePath $ip] && ![info exists uniqueImplIPs($id)] } {
# Updated
# Goal: avoid errors caused by IP names (in case of multiple IPs, files copied into the IP_CACHE might have different names as RW expects. Possible explanation = RW reads IPs in this loop in a different order as it copies the .dcps already generated by vivado )
if {[get_uniq_ip_name $cachePath $ip]!="none"} {
set ip [get_uniq_ip_name $cachePath $ip]
}
# End of updated
puts "OPT RUN: $ip"
lappend opt_runs_needed $ip
set uniqueImplIPs($id) $ip
Expand Down Expand Up @@ -662,3 +673,25 @@ proc offset_dsps { count } {
puts "Moving $c from $s to $new_site"
}
}

# Updated
# Goal: Get name of ip based on the name of the files copied into the directory with the corresponding ID. Useful for designs using multiple IPs, in order to avoid naming errors after copying files into the IP_CACHE
proc get_uniq_ip_name {cachePath ip} {
set cache_id [config_ip_cache -get_id $ip]
set cacheIPDir "${cachePath}[cache_version_dir]/$cache_id"
if { ! [file exists $cacheIPDir] } {
puts "ERROR! No IP in this folder"
return "none"
}
set existingIP [lindex [get_lines_matching instanceName ${cacheIPDir}/${cache_id}.xci] 0]
set existingIP [string map {"<spirit:instanceName>" ""} $existingIP]
set existingIP [string map {"</spirit:instanceName>" ""} $existingIP]
set existingIP [string trim $existingIP]
set ip_return [get_ips $existingIP]
if {$ip_return!={}} {
return $ip_return
} else {
return $ip
}
}
# End of updated
Copy link
Member

Choose a reason for hiding this comment

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

Looks good, perhaps just add a newline at the end of the file.