<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Feb 26, 2018 at 2:42 PM, Clement Courbet <span dir="ltr"><<a href="mailto:courbet@google.com" target="_blank">courbet@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="gmail-">On Mon, Feb 26, 2018 at 3:37 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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><span><snip></span><br></div><div class="gmail_extra"><div class="gmail_quote"><div>As I wrote before, I will be sending an RFC where I present my tool, goals, design, limitations and issues found (some of which still require a proper solution). If we see that the two tools are going to be semantically the same, then the RFC thread is a good place where to talk (and plan) things.<br></div></div></div></div></blockquote><div><br></div></span><div>Sounds good.</div></div></div></div></blockquote><div><br></div><div>FYI,<br>I posted the RFC on llvm-dev. You should have received an email. Here is the link: <a href="http://lists.llvm.org/pipermail/llvm-dev/2018-March/121490.html">http://lists.llvm.org/pipermail/llvm-dev/2018-March/121490.html</a><br><br></div><div>Cheers,<br></div><div>Andrea<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="gmail-"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>I will also be at the conference. So, we can definitely have a chat there too :-).<br></div></div></div></div></blockquote><div><br></div></span><div>Perfect !</div><div><div class="gmail-h5"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><span>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></span>[...]<span><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></span></div></blockquote><div><br></div><div>The reason I went in that direction is the 64 resource limit; my initial approach was the same as yours. I don't have strong feelings about which is better.</div><div>I agree with the set operations being easier (my tool actually uses BitVector internally), on the other the table approach is more general and having an extra table hurts because it's super tiny.</div></div></div></div></blockquote><div><br></div></span><div>To be honest I don't have a strong opinion about this. Indeed, bitmasks would make my life easier in general because those are easier to manipulate. I am not worried about the resource limit, because realistically the number of bit (64) is going to be more than enough in practice for most (all?) targets. but I also agree that there is nothing wrong with having an extra table.<br></div><div>In terms of design, both approaches are good. I don't think it is a problem for my tool to convert the information from that table into masks (it should be very simple and fast to do it).<br></div><span><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>I have no issue going in the bitmask direction if that works better for you and other people; the semantics are the same.</div><div>Feel free to send me a diff.</div></div></div></div></blockquote><div><br></div></span><div>For now, it is probably better to stick with your approach. Once I shared my RFC with you, and we have a clear idea in mind on what to do next, then we may revisit this approach (if it sounds good to you). This may be something that we can also talk about at the conference maybe.<br><br></div><div>Cheers,<br></div><div>Andrea<br><br></div><div><div class="gmail-m_2753027245227468306h5"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="gmail-m_2753027245227468306m_8560149887699515399h5"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">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="gmail-m_2753027245227468306m_8560149887699515399m_8464095954762741855gmail-HOEnZb"><div class="gmail-m_2753027245227468306m_8560149887699515399m_8464095954762741855gmail-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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Yes, thanks for pointing that out.</div><div class="gmail-m_2753027245227468306m_8560149887699515399m_8464095954762741855gmail-m_-3858478937597783393HOEnZb"><div class="gmail-m_2753027245227468306m_8560149887699515399m_8464095954762741855gmail-m_-3858478937597783393h5"><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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);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="gmail-m_2753027245227468306m_8560149887699515399m_8464095954762741855gmail-m_-3858478937597783393m_-293534449721402970HOEnZb"><font color="#888888"><br clear="all"><div><div class="gmail-m_2753027245227468306m_8560149887699515399m_8464095954762741855gmail-m_-3858478937597783393m_-293534449721402970m_8675278859987401302gmail_signature">~Craig</div></div></font></span><div><div class="gmail-m_2753027245227468306m_8560149887699515399m_8464095954762741855gmail-m_-3858478937597783393m_-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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);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></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>