[llvm] r324582 - [TargetSchedule] Expose sub-units of a ProcResGroup in MCProcResourceDesc.
Andrea Di Biagio via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 1 09:38:07 PST 2018
On Mon, Feb 26, 2018 at 2:42 PM, Clement Courbet <courbet at google.com> wrote:
>
>
> On Mon, Feb 26, 2018 at 3:37 PM, Andrea Di Biagio <
> andrea.dibiagio at gmail.com> wrote:
>
>>
>> <snip>
>> 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.
>
FYI,
I posted the RFC on llvm-dev. You should have received an email. Here is
the link: http://lists.llvm.org/pipermail/llvm-dev/2018-March/121490.html
Cheers,
Andrea
>
>
>> 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/20180301/b23c2842/attachment.html>
More information about the llvm-commits
mailing list