<div dir="ltr"><div>Ping.<br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Feb 19, 2018 at 5:23 PM, Andrea Di Biagio <span dir="ltr"><<a href="mailto:andrea.dibiagio@gmail.com" target="_blank">andrea.dibiagio@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi Clement,<br><br>Sorry for the late reply. I have been off sick for a while and I've only just seen your commit now I'm back to work.<br><br>I am currently working on an LLVM based performance analysis tool which reuses the information from the scheduling model, for which I plan to send an RFC soon (next week hopefully).<br><br>When working on my tool, I had to solve a similar problem to this, and had come up with a different fix which doesn't require generating an extra table with indices.<br><br>My implementation (for which I'm happy to share the patch) uses simple bitmasks to solve set membership problems. The idea is that each processor resource is associated with a bitmask value (a 64-bit unisigned quantity) which can be used as a unique identifier.<br><br>Example:<br><br> ResourceA 001<br> ResourceB 010<br> ResourceAB 111<br><br>Here, Processor resource units only have one bit set. So we can easily tell if a resource is a group or not by simply checking the popcount of the bitmask. Resource groups will always have more than one bit set in the mask, while normal resources would always have exactly one bit set.<br><br>Each mask is unique, and we can use the mask value to quickly check if a unit is part of a group. Back to the example, ResourceAB has three bits set. If we exclude the most significant bit, the remaining bits describe the composition of the set.<br>In this case we have that:<br> 111 --> 100 U (010 U 001)<br><br>There is a precedent for doing things like this. For example, functional units in itineraries are uniquely identified by an unsigned value (see for example AtomItinerariesFU table in the x86 backend).<br><br>The advantage is that:<br> a) we don't require an extra table,<br> b) we can use the mask both as an identifier, and to quickly solve set membership problems (with simple bit manipulation operations), and<br> c) (as a consequence of b) we can simplify some of the logic that verifies the composition of resource groups (for example, method CodeGenSchedModels::<wbr>verifyProcResourceGroups in CodeGenSchedule.cpp may be a good candidate).<br><br>Since my approach uses bitmasks, there is a limit in the number of resources that can be described. With 64 bits we are obviously limited to having 64 resources maximum.<br>That being said, the processors with the most resources that I have observed in the opensource are Haswell and Broadwell which declare 22 unique resources each (far below what can be encoded with a 64-bit mask).<br><br>I tried to look to see if there is any specific reason why this approach was taken in the review, but I couldn't see it there. If there is not a specific reason to use the table approach, would you be okay if we adopted the bitmask approach?<br>To be clear, I am not suggesting that your approach is wrong. However, I just want to point out that there may be potentially better approaches.<br>The bitmask approach makes the implementation of my tool easier (it would make my life easier :-)).<br><br>Thanks,<br>Andrea<br></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Feb 9, 2018 at 8:17 AM, Clement Courbet via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Yes, thanks for pointing that out.</div><div class="m_-842593942566433936HOEnZb"><div class="m_-842593942566433936h5"><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Feb 8, 2018 at 7:33 PM, Craig Topper <span dir="ltr"><<a href="mailto:craig.topper@gmail.com" target="_blank">craig.topper@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Is this responsible for this warning I'm seeing now<div><br></div><div><div>lib/Target/X86/X86GenSubtarget<wbr>Info.inc:52484:39: warning: missing field 'SubUnitsIdxBegin' initializer [-Wmissing-field-initializers]</div><span><div> {DBGFIELD("InvalidUnit") 0, 0, 0},</div></span></div></div><div class="gmail_extra"><span class="m_-842593942566433936m_-293534449721402970HOEnZb"><font color="#888888"><br clear="all"><div><div class="m_-842593942566433936m_-293534449721402970m_8675278859987401302gmail_signature" data-smartmail="gmail_signature">~Craig</div></div></font></span><div><div class="m_-842593942566433936m_-293534449721402970h5">
<br><div class="gmail_quote">On Thu, Feb 8, 2018 at 12:46 AM, Clement Courbet via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: courbet<br>
Date: Thu Feb 8 00:46:48 2018<br>
New Revision: 324582<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=324582&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject?rev=324582&view=rev</a><br>
Log:<br>
[TargetSchedule] Expose sub-units of a ProcResGroup in MCProcResourceDesc.<br>
<br>
Summary:<br>
Right now using a ProcResource automatically counts as usage of all<br>
super ProcResGroups. All this is done during codegen, so there is no<br>
way for schedulers to get this information at runtime.<br>
<br>
This adds the information of which individual ProcRes units are<br>
contained in a ProcResGroup in MCProcResourceDesc.<br>
<br>
Reviewers: gchatelet<br>
<br>
Subscribers: llvm-commits<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D43023" rel="noreferrer" target="_blank">https://reviews.llvm.org/D4302<wbr>3</a><br>
<br>
Modified:<br>
llvm/trunk/include/llvm/MC/MCS<wbr>chedule.h<br>
llvm/trunk/include/llvm/Target<wbr>/TargetSchedule.td<br>
llvm/trunk/utils/TableGen/Subt<wbr>argetEmitter.cpp<br>
<br>
Modified: llvm/trunk/include/llvm/MC/MCS<wbr>chedule.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/MC/MCSchedule.h?rev=324582&r1=324581&r2=324582&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/llvm/trunk/include/llvm/<wbr>MC/MCSchedule.h?rev=324582&r1=<wbr>324581&r2=324582&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/include/llvm/MC/MCS<wbr>chedule.h (original)<br>
+++ llvm/trunk/include/llvm/MC/MCS<wbr>chedule.h Thu Feb 8 00:46:48 2018<br>
@@ -44,6 +44,11 @@ struct MCProcResourceDesc {<br>
// an out-of-order cpus.<br>
int BufferSize;<br>
<br>
+ // If the resource has sub-units, a pointer to the first element of an array<br>
+ // of `NumUnits` elements containing the ProcResourceIdx of the sub units.<br>
+ // nullptr if the resource does not have sub-units.<br>
+ const unsigned *SubUnitsIdxBegin;<br>
+<br>
bool operator==(const MCProcResourceDesc &Other) const {<br>
return NumUnits == Other.NumUnits && SuperIdx == Other.SuperIdx<br>
&& BufferSize == Other.BufferSize;<br>
<br>
Modified: llvm/trunk/include/llvm/Target<wbr>/TargetSchedule.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Target/TargetSchedule.td?rev=324582&r1=324581&r2=324582&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/llvm/trunk/include/llvm/<wbr>Target/TargetSchedule.td?rev=3<wbr>24582&r1=324581&r2=324582&view<wbr>=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/include/llvm/Target<wbr>/TargetSchedule.td (original)<br>
+++ llvm/trunk/include/llvm/Target<wbr>/TargetSchedule.td Thu Feb 8 00:46:48 2018<br>
@@ -175,8 +175,7 @@ class ProcResourceKind;<br>
// BufferSize=1.<br>
//<br>
// SchedModel ties these units to a processor for any stand-alone defs<br>
-// of this class. Instances of subclass ProcResource will be automatically<br>
-// attached to a processor, so SchedModel is not needed.<br>
+// of this class.<br>
class ProcResourceUnits<ProcResource<wbr>Kind kind, int num> {<br>
ProcResourceKind Kind = kind;<br>
int NumUnits = num;<br>
<br>
Modified: llvm/trunk/utils/TableGen/Subt<wbr>argetEmitter.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/utils/TableGen/SubtargetEmitter.cpp?rev=324582&r1=324581&r2=324582&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/llvm/trunk/utils/TableGe<wbr>n/SubtargetEmitter.cpp?rev=324<wbr>582&r1=324581&r2=324582&view=d<wbr>iff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/utils/TableGen/Subt<wbr>argetEmitter.cpp (original)<br>
+++ llvm/trunk/utils/TableGen/Subt<wbr>argetEmitter.cpp Thu Feb 8 00:46:48 2018<br>
@@ -92,6 +92,8 @@ class SubtargetEmitter {<br>
&ProcItinLists);<br>
void EmitProcessorProp(raw_ostream &OS, const Record *R, StringRef Name,<br>
char Separator);<br>
+ void EmitProcessorResourceSubUnits(<wbr>const CodeGenProcModel &ProcModel,<br>
+ raw_ostream &OS);<br>
void EmitProcessorResources(const CodeGenProcModel &ProcModel,<br>
raw_ostream &OS);<br>
Record *FindWriteResources(const CodeGenSchedRW &SchedWrite,<br>
@@ -578,24 +580,52 @@ void SubtargetEmitter::EmitProcesso<wbr>rProp<br>
OS << '\n';<br>
}<br>
<br>
+void SubtargetEmitter::EmitProcesso<wbr>rResourceSubUnits(<br>
+ const CodeGenProcModel &ProcModel, raw_ostream &OS) {<br>
+ OS << "\nstatic const unsigned " << ProcModel.ModelName<br>
+ << "ProcResourceSubUnits[] = {\n"<br>
+ << " 0, // Invalid\n";<br>
+<br>
+ for (unsigned i = 0, e = ProcModel.ProcResourceDefs.siz<wbr>e(); i < e; ++i) {<br>
+ Record *PRDef = ProcModel.ProcResourceDefs[i];<br>
+ if (!PRDef->isSubClassOf("ProcRes<wbr>Group"))<br>
+ continue;<br>
+ RecVec ResUnits = PRDef->getValueAsListOfDefs("R<wbr>esources");<br>
+ for (Record *RUDef : ResUnits) {<br>
+ Record *const RU =<br>
+ SchedModels.findProcResUnits(R<wbr>UDef, ProcModel, PRDef->getLoc());<br>
+ for (unsigned J = 0; J < RU->getValueAsInt("NumUnits"); ++J) {<br>
+ OS << " " << ProcModel.getProcResourceIdx(R<wbr>U) << ", ";<br>
+ }<br>
+ }<br>
+ OS << " // " << PRDef->getName() << "\n";<br>
+ }<br>
+ OS << "};\n";<br>
+}<br>
+<br>
void SubtargetEmitter::EmitProcesso<wbr>rResources(const CodeGenProcModel &ProcModel,<br>
raw_ostream &OS) {<br>
- OS << "\n// {Name, NumUnits, SuperIdx, IsBuffered}\n";<br>
+ EmitProcessorResourceSubUnits(<wbr>ProcModel, OS);<br>
+<br>
+ OS << "\n// {Name, NumUnits, SuperIdx, IsBuffered, SubUnitsIdxBegin}\n";<br>
OS << "static const llvm::MCProcResourceDesc "<br>
<< ProcModel.ModelName << "ProcResources" << "[] = {\n"<br>
<< " {DBGFIELD(\"InvalidUnit\") 0, 0, 0},\n";<br>
<br>
+ unsigned SubUnitsOffset = 1;<br>
for (unsigned i = 0, e = ProcModel.ProcResourceDefs.siz<wbr>e(); i < e; ++i) {<br>
Record *PRDef = ProcModel.ProcResourceDefs[i];<br>
<br>
Record *SuperDef = nullptr;<br>
unsigned SuperIdx = 0;<br>
unsigned NumUnits = 0;<br>
+ const unsigned SubUnitsBeginOffset = SubUnitsOffset;<br>
int BufferSize = PRDef->getValueAsInt("BufferSi<wbr>ze");<br>
if (PRDef->isSubClassOf("ProcResG<wbr>roup")) {<br>
RecVec ResUnits = PRDef->getValueAsListOfDefs("R<wbr>esources");<br>
for (Record *RU : ResUnits) {<br>
NumUnits += RU->getValueAsInt("NumUnits");<br>
+ SubUnitsOffset += NumUnits;<br>
}<br>
}<br>
else {<br>
@@ -612,8 +642,14 @@ void SubtargetEmitter::EmitProcesso<wbr>rReso<br>
OS << " {DBGFIELD(\"" << PRDef->getName() << "\") ";<br>
if (PRDef->getName().size() < 15)<br>
OS.indent(15 - PRDef->getName().size());<br>
- OS << NumUnits << ", " << SuperIdx << ", "<br>
- << BufferSize << "}, // #" << i+1;<br>
+ OS << NumUnits << ", " << SuperIdx << ", " << BufferSize << ", ";<br>
+ if (SubUnitsBeginOffset != SubUnitsOffset) {<br>
+ OS << ProcModel.ModelName << "ProcResourceSubUnits + "<br>
+ << SubUnitsBeginOffset;<br>
+ } else {<br>
+ OS << "nullptr";<br>
+ }<br>
+ OS << "}, // #" << i+1;<br>
if (SuperDef)<br>
OS << ", Super=" << SuperDef->getName();<br>
OS << "\n";<br>
<br>
<br>
______________________________<wbr>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div></div>
</blockquote></div><br></div>
</div></div><br>______________________________<wbr>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>
</div></div></blockquote></div><br></div>