[llvm] r324582 - [TargetSchedule] Expose sub-units of a ProcResGroup in MCProcResourceDesc.
Clement Courbet via llvm-commits
llvm-commits at lists.llvm.org
Mon Feb 26 06:42:38 PST 2018
On Mon, Feb 26, 2018 at 3:37 PM, Andrea Di Biagio <andrea.dibiagio at gmail.com
> wrote:
>
>
> On Mon, Feb 26, 2018 at 12:30 PM, Clement Courbet <courbet at google.com>
> wrote:
>
>> Hi Andrea, sorry I missed your email.
>>
>> On Mon, Feb 19, 2018 at 6:23 PM, Andrea Di Biagio <
>> andrea.dibiagio at gmail.com> wrote:
>>
>>> Hi Clement,
>>>
>>> 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.
>>>
>>> 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).
>>>
>>
>> Interesting, I'm working on a similar thing and I'm going to give a talk
>> about it at EuroLLVM next month. Please keep me in the loop :)
>>
>
> Interesting. I am working on a similar tool too...
> I suspect that there is a big overlap between our projects :-P.
>
> 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.
>
Sounds good.
> I will also be at the conference. So, we can definitely have a chat there
> too :-).
>
Perfect !
>>
>>> 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.
>>>
>>> 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.
>>> [...]
>>> 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?
>>>
>>
>> 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.
>> 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.
>>
>
> 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.
> 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).
>
> I have no issue going in the bitmask direction if that works better for
>> you and other people; the semantics are the same.
>> Feel free to send me a diff.
>>
>
> 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.
>
> Cheers,
> Andrea
>
>
>>
>>> 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.
>>> The bitmask approach makes the implementation of my tool easier (it
>>> would make my life easier :-)).
>>>
>>> Thanks,
>>> Andrea
>>>
>>> On Fri, Feb 9, 2018 at 8:17 AM, Clement Courbet via llvm-commits <
>>> llvm-commits at lists.llvm.org> wrote:
>>>
>>>> Yes, thanks for pointing that out.
>>>>
>>>> On Thu, Feb 8, 2018 at 7:33 PM, Craig Topper <craig.topper at gmail.com>
>>>> wrote:
>>>>
>>>>> Is this responsible for this warning I'm seeing now
>>>>>
>>>>> lib/Target/X86/X86GenSubtargetInfo.inc:52484:39: warning: missing
>>>>> field 'SubUnitsIdxBegin' initializer [-Wmissing-field-initializers]
>>>>> {DBGFIELD("InvalidUnit") 0, 0, 0},
>>>>>
>>>>> ~Craig
>>>>>
>>>>> On Thu, Feb 8, 2018 at 12:46 AM, Clement Courbet via llvm-commits <
>>>>> llvm-commits at lists.llvm.org> wrote:
>>>>>
>>>>>> Author: courbet
>>>>>> Date: Thu Feb 8 00:46:48 2018
>>>>>> New Revision: 324582
>>>>>>
>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=324582&view=rev
>>>>>> Log:
>>>>>> [TargetSchedule] Expose sub-units of a ProcResGroup in
>>>>>> MCProcResourceDesc.
>>>>>>
>>>>>> Summary:
>>>>>> Right now using a ProcResource automatically counts as usage of all
>>>>>> super ProcResGroups. All this is done during codegen, so there is no
>>>>>> way for schedulers to get this information at runtime.
>>>>>>
>>>>>> This adds the information of which individual ProcRes units are
>>>>>> contained in a ProcResGroup in MCProcResourceDesc.
>>>>>>
>>>>>> Reviewers: gchatelet
>>>>>>
>>>>>> Subscribers: llvm-commits
>>>>>>
>>>>>> Differential Revision: https://reviews.llvm.org/D43023
>>>>>>
>>>>>> Modified:
>>>>>> llvm/trunk/include/llvm/MC/MCSchedule.h
>>>>>> llvm/trunk/include/llvm/Target/TargetSchedule.td
>>>>>> llvm/trunk/utils/TableGen/SubtargetEmitter.cpp
>>>>>>
>>>>>> Modified: llvm/trunk/include/llvm/MC/MCSchedule.h
>>>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/
>>>>>> MC/MCSchedule.h?rev=324582&r1=324581&r2=324582&view=diff
>>>>>> ============================================================
>>>>>> ==================
>>>>>> --- llvm/trunk/include/llvm/MC/MCSchedule.h (original)
>>>>>> +++ llvm/trunk/include/llvm/MC/MCSchedule.h Thu Feb 8 00:46:48 2018
>>>>>> @@ -44,6 +44,11 @@ struct MCProcResourceDesc {
>>>>>> // an out-of-order cpus.
>>>>>> int BufferSize;
>>>>>>
>>>>>> + // If the resource has sub-units, a pointer to the first element
>>>>>> of an array
>>>>>> + // of `NumUnits` elements containing the ProcResourceIdx of the
>>>>>> sub units.
>>>>>> + // nullptr if the resource does not have sub-units.
>>>>>> + const unsigned *SubUnitsIdxBegin;
>>>>>> +
>>>>>> bool operator==(const MCProcResourceDesc &Other) const {
>>>>>> return NumUnits == Other.NumUnits && SuperIdx == Other.SuperIdx
>>>>>> && BufferSize == Other.BufferSize;
>>>>>>
>>>>>> Modified: llvm/trunk/include/llvm/Target/TargetSchedule.td
>>>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/
>>>>>> Target/TargetSchedule.td?rev=324582&r1=324581&r2=324582&view=diff
>>>>>> ============================================================
>>>>>> ==================
>>>>>> --- llvm/trunk/include/llvm/Target/TargetSchedule.td (original)
>>>>>> +++ llvm/trunk/include/llvm/Target/TargetSchedule.td Thu Feb 8
>>>>>> 00:46:48 2018
>>>>>> @@ -175,8 +175,7 @@ class ProcResourceKind;
>>>>>> // BufferSize=1.
>>>>>> //
>>>>>> // SchedModel ties these units to a processor for any stand-alone
>>>>>> defs
>>>>>> -// of this class. Instances of subclass ProcResource will be
>>>>>> automatically
>>>>>> -// attached to a processor, so SchedModel is not needed.
>>>>>> +// of this class.
>>>>>> class ProcResourceUnits<ProcResourceKind kind, int num> {
>>>>>> ProcResourceKind Kind = kind;
>>>>>> int NumUnits = num;
>>>>>>
>>>>>> Modified: llvm/trunk/utils/TableGen/SubtargetEmitter.cpp
>>>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/utils/TableGe
>>>>>> n/SubtargetEmitter.cpp?rev=324582&r1=324581&r2=324582&view=diff
>>>>>> ============================================================
>>>>>> ==================
>>>>>> --- llvm/trunk/utils/TableGen/SubtargetEmitter.cpp (original)
>>>>>> +++ llvm/trunk/utils/TableGen/SubtargetEmitter.cpp Thu Feb 8
>>>>>> 00:46:48 2018
>>>>>> @@ -92,6 +92,8 @@ class SubtargetEmitter {
>>>>>> &ProcItinLists);
>>>>>> void EmitProcessorProp(raw_ostream &OS, const Record *R, StringRef
>>>>>> Name,
>>>>>> char Separator);
>>>>>> + void EmitProcessorResourceSubUnits(const CodeGenProcModel
>>>>>> &ProcModel,
>>>>>> + raw_ostream &OS);
>>>>>> void EmitProcessorResources(const CodeGenProcModel &ProcModel,
>>>>>> raw_ostream &OS);
>>>>>> Record *FindWriteResources(const CodeGenSchedRW &SchedWrite,
>>>>>> @@ -578,24 +580,52 @@ void SubtargetEmitter::EmitProcessorProp
>>>>>> OS << '\n';
>>>>>> }
>>>>>>
>>>>>> +void SubtargetEmitter::EmitProcessorResourceSubUnits(
>>>>>> + const CodeGenProcModel &ProcModel, raw_ostream &OS) {
>>>>>> + OS << "\nstatic const unsigned " << ProcModel.ModelName
>>>>>> + << "ProcResourceSubUnits[] = {\n"
>>>>>> + << " 0, // Invalid\n";
>>>>>> +
>>>>>> + for (unsigned i = 0, e = ProcModel.ProcResourceDefs.size(); i <
>>>>>> e; ++i) {
>>>>>> + Record *PRDef = ProcModel.ProcResourceDefs[i];
>>>>>> + if (!PRDef->isSubClassOf("ProcResGroup"))
>>>>>> + continue;
>>>>>> + RecVec ResUnits = PRDef->getValueAsListOfDefs("Resources");
>>>>>> + for (Record *RUDef : ResUnits) {
>>>>>> + Record *const RU =
>>>>>> + SchedModels.findProcResUnits(RUDef, ProcModel,
>>>>>> PRDef->getLoc());
>>>>>> + for (unsigned J = 0; J < RU->getValueAsInt("NumUnits"); ++J) {
>>>>>> + OS << " " << ProcModel.getProcResourceIdx(RU) << ", ";
>>>>>> + }
>>>>>> + }
>>>>>> + OS << " // " << PRDef->getName() << "\n";
>>>>>> + }
>>>>>> + OS << "};\n";
>>>>>> +}
>>>>>> +
>>>>>> void SubtargetEmitter::EmitProcessorResources(const
>>>>>> CodeGenProcModel &ProcModel,
>>>>>> raw_ostream &OS) {
>>>>>> - OS << "\n// {Name, NumUnits, SuperIdx, IsBuffered}\n";
>>>>>> + EmitProcessorResourceSubUnits(ProcModel, OS);
>>>>>> +
>>>>>> + OS << "\n// {Name, NumUnits, SuperIdx, IsBuffered,
>>>>>> SubUnitsIdxBegin}\n";
>>>>>> OS << "static const llvm::MCProcResourceDesc "
>>>>>> << ProcModel.ModelName << "ProcResources" << "[] = {\n"
>>>>>> << " {DBGFIELD(\"InvalidUnit\") 0, 0, 0},\n";
>>>>>>
>>>>>> + unsigned SubUnitsOffset = 1;
>>>>>> for (unsigned i = 0, e = ProcModel.ProcResourceDefs.size(); i <
>>>>>> e; ++i) {
>>>>>> Record *PRDef = ProcModel.ProcResourceDefs[i];
>>>>>>
>>>>>> Record *SuperDef = nullptr;
>>>>>> unsigned SuperIdx = 0;
>>>>>> unsigned NumUnits = 0;
>>>>>> + const unsigned SubUnitsBeginOffset = SubUnitsOffset;
>>>>>> int BufferSize = PRDef->getValueAsInt("BufferSize");
>>>>>> if (PRDef->isSubClassOf("ProcResGroup")) {
>>>>>> RecVec ResUnits = PRDef->getValueAsListOfDefs("Resources");
>>>>>> for (Record *RU : ResUnits) {
>>>>>> NumUnits += RU->getValueAsInt("NumUnits");
>>>>>> + SubUnitsOffset += NumUnits;
>>>>>> }
>>>>>> }
>>>>>> else {
>>>>>> @@ -612,8 +642,14 @@ void SubtargetEmitter::EmitProcessorReso
>>>>>> OS << " {DBGFIELD(\"" << PRDef->getName() << "\") ";
>>>>>> if (PRDef->getName().size() < 15)
>>>>>> OS.indent(15 - PRDef->getName().size());
>>>>>> - OS << NumUnits << ", " << SuperIdx << ", "
>>>>>> - << BufferSize << "}, // #" << i+1;
>>>>>> + OS << NumUnits << ", " << SuperIdx << ", " << BufferSize << ", ";
>>>>>> + if (SubUnitsBeginOffset != SubUnitsOffset) {
>>>>>> + OS << ProcModel.ModelName << "ProcResourceSubUnits + "
>>>>>> + << SubUnitsBeginOffset;
>>>>>> + } else {
>>>>>> + OS << "nullptr";
>>>>>> + }
>>>>>> + OS << "}, // #" << i+1;
>>>>>> if (SuperDef)
>>>>>> OS << ", Super=" << SuperDef->getName();
>>>>>> OS << "\n";
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> llvm-commits mailing list
>>>>>> llvm-commits at lists.llvm.org
>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>>
>>>>>
>>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180226/f1c256a8/attachment-0001.html>
More information about the llvm-commits
mailing list