This is an archive of the discontinued LLVM Phabricator instance.

[llvm-mca] Avoid exposing index values in the MCA interfaces.
ClosedPublic

Authored by mattd on May 2 2018, 3:26 PM.

Details

Summary

This patch eliminates many places where we originally needed to pass index
values to represent an instruction. The index is still used as a key, in various parts of
MCA. I'm not comfortable eliminating the index just yet. By burying the index in
the instruction, we can avoid exposing that value in many places.

Eventually, we should consider removing the Instructions list in the Backend
all together, it's only used to hold and reclaim the memory for the allocated
Instruction instances. Instead we could pass around a smart pointer. But that's
a separate discussion/patch.

Diff Detail

Event Timeline

mattd created this revision.May 2 2018, 3:26 PM
mattd edited the summary of this revision. (Show Details)May 2 2018, 3:28 PM
mattd updated this revision to Diff 144965.May 2 2018, 6:46 PM
  • Remove index values from the LSUnit routines. Essentially more of what this patch is trying to accomplish.

Hi Matt,

what about passing an InstRef (i.e. a pair<index, Instruction>) to all those APIs?
I would not need to add a field to Instruction. The index is a SourceMgr concept. The only reason why those functionalities require it is mainly because they have to notify events, and events are based on the instruction index.

So, you could have that methods like checkRAT/checkXXX/etc. now accept an InstRef instead of a "index, Instruction" or (even worse) "index, InstrDesc". I would avout to change the Event classes if possible (at least for now).

About the instruction ownership issue you mentioned in your last paragraph: I am curious to see if it works the idea of having the RCU as the owner. An instruction exists if it is in-flight in the OoO backend. So, it must be known by the RCU through its entire lifetime. You could do some experiments to see if that idea works.

mattd added a comment.May 4 2018, 9:22 AM

Hi Matt,

what about passing an InstRef (i.e. a pair<index, Instruction>) to all those APIs?
I would not need to add a field to Instruction. The index is a SourceMgr concept. The only reason why those functionalities require it is mainly because they have to notify events, and events are based on the instruction index.

So, you could have that methods like checkRAT/checkXXX/etc. now accept an InstRef instead of a "index, Instruction" or (even worse) "index, InstrDesc". I would avout to change the Event classes if possible (at least for now).

Thanks for the feedback, and I see your point. Eventually, I'd like to eliminate the Index entirely. I suppose we could consider this change an intermediate step towards the goal. I was originally aiming at removing the Owner pointers in a few places. At least, for the cases where the Owner was only needed to perform an index-to-instruction lookup.

About the instruction ownership issue you mentioned in your last paragraph: I am curious to see if it works the idea of having the RCU as the owner. An instruction exists if it is in-flight in the OoO backend. So, it must be known by the RCU through its entire lifetime. You could do some experiments to see if that idea works.

I think that will work. Since the RCU will hold a reference to the Instruction, by design it would force the object to remain alive. Once the RCU finishes using the Instruction, then it will automatically be reclaimed.

mattd updated this revision to Diff 145393.May 5 2018, 9:44 PM

Respin of the previous patch:

  • InstRef is now a pair <SourceMgr index, Instruction *>, with added sanity checks and convenience accessors.
  • SourceRef is a new type and defined as a pair <SourceMgr index, MCInst>

I'm not sure this is ideal, yet, but it is one step closer to sharing a common interface across various MCA components. The previous patch already updated the Event interface. I kept those changes, as it eliminates the need to have an Owner pointer for lookup in those cases, since we now have the InstRef containing the Instruction that was needed.

Hi Matt,

This diff has not been taken against top of trunk. Could you please rebase the patch and then create a correct diff?
For example, class Instuction never had a field named SourceIndex!

Also, see my inline comments.

Thanks,
Andrea

tools/llvm-mca/Dispatch.cpp
329

Use an instruction reference here.
Instruction &IS = *IR.getInstruction();

tools/llvm-mca/InstrBuilder.cpp
429–431

Same. You should rebase against ToT.

tools/llvm-mca/InstrBuilder.h
62 ↗(On Diff #145393)

Please correctly rebase this patch. This change should not have been here.

tools/llvm-mca/Instruction.h
20

Please remove this. See my comment below.
Essentially, raw_ostream is only used for debugging purposes here. Move the printing functionality to the cpp file, and guard the declaration of the printing functionality using a check on NDEBUG. This include can then be removed.

40

What's the advantage in having this constructor? I think you can remove it.
All the instruction references should be constructed using the other constructor at line 42.

47

Not needed. It is okay to return nullptr.

52

Not needed.

59–67

This code should be guarded against NDEBUG.
Everywhere else in the codebase we use method dump() to do debug prints. You should do the same, and move the definition in Instruction.cpp.

tools/llvm-mca/RetireControlUnit.cpp
31

Why a pointer? I think InstRef should always be passed by reference or by copy.
This diff doesn't match Upstream! The signature should have been different. reserveSlot accepts an instruction index, and not an instruction reference!

40

This is wrong.

57

Can this ever happen?

tools/llvm-mca/RetireControlUnit.h
50

It was an unsigned integer before. Now it is an InstRef. Please explain why?

77

This is incorrect. Regardless of the wrong diff.

tools/llvm-mca/Scheduler.cpp
415–416
const InstrDesc &D = Entry.second->getDesc();
andreadb requested changes to this revision.May 6 2018, 3:23 AM
This revision now requires changes to proceed.May 6 2018, 3:23 AM
mattd marked 5 inline comments as done.May 6 2018, 11:44 AM

Thanks for the review. Sorry for building on the previous patch, I should have rebased against ToT.

tools/llvm-mca/InstrBuilder.h
62 ↗(On Diff #145393)

Sorry about that, I was just playing off of the previous patch for this that I was working on.

tools/llvm-mca/Instruction.h
40

I was allowing for the case of storing of InstRef objects in a container, not by reference or pointer. This would only be for the RCU's queue. But perhaps we remove this default ctor, and just store an Instruction pointer in the RUToken. I've discussed this more in a comment below.

47

Ok, I'll remove it. I was trying to detect any states that should not arise, but I wanted to guarantee that they did not occur. If, for instance, an InstRef was constructed with a nullptr value for the Instruction. Or some default construction never getting populated. But, as I mentioned in a comment above, and as you suggested, we can get rid of the default ctor. But that is somewhat dependent on how we define the RUToken, which is discussed in a different comment below.

59–67

I agree with the NDEBUG.

This overload is just providing a convenient way to pass an InstRef to an output stream. We were already passing index values to our debug output. Since we are now passing around InstRef in most places, I figured it would be nice to overload the output operator for InstRef, so that our existing debug prints would not have to call "getSourceIndex."

tools/llvm-mca/RetireControlUnit.cpp
31

Did you want to keep the original definition of reserveSlot, by taking an index? Since we are simplifying the interface where most routines take an InstRef, I think passing a reference would make sense, as it is later used for event notification. I was hoping to remove all of the Owner->getInstruction() lookups.

57

I'm not sure, so I figured it was a reasonable assertion to make.

tools/llvm-mca/RetireControlUnit.h
50

The reason is that the token is eventually used to signal that a particular instruction has retired, the call to notifyInstructionRetired in RetireControlUnit::cycleEvent. I was trying to avoid storing an index, since the notification mechanism would have to eventually perform a lookup of that index to get the instruction pointer. I was trying to decouple eventing from the component containing all of the instruction instances, I do realize that RCU will probably manage ownership of all Instructions at some point. I was trying to avoid having one component query another MCA component to get an instruction. We already have the instruction when we generate the RUToken values, so my thought was just to pass that information instead.

77

I agree, it shouldn't be a pointer. I'm not sure why I changed that, other than it was how things fell out from the previous patch. Sorry. I'll fix that.

Thanks Matt,

tools/llvm-mca/InstrBuilder.h
62 ↗(On Diff #145393)

No problem. It happens :-)

tools/llvm-mca/Instruction.h
59–67

Makes sense. Thanks.

tools/llvm-mca/RetireControlUnit.cpp
31

That’s okay. I was just confused by the presence of a pointer.

tools/llvm-mca/RetireControlUnit.h
50

Thanks for the explanation. Makes sense :-).

mattd updated this revision to Diff 145423.May 6 2018, 8:34 PM
mattd marked 6 inline comments as done and an inline comment as not done.

Updated this patch per some of the comments discussed:

  • Rebased against the ToT.
  • Added an NDEBUG guard to the debug output routine. I wanted to keep this routine to eliminate calls to getSourceIndex in our debug output. This allows us to keep the syntax in debug output code clean.
  • Made a pointer a const ref for the formal parameter in RCU's reserveSlot(). I'm not sure why it was treated as a pointer previously. I have a feeling my earlier version of this patch was tossing around a Instruction * in that case, and I never adjusted for that.
  • I wanted to remove the copy of the InstRef in the RUToken queue. That queue is a std::vector which we resize. This container requires a default InstRef ctor.

These changes allow us to partially decouple the RCU and Backend, in other words I've removed the Backend's "getInstruction" function. Only one eventing routine is still coupled to an index value, which couples the RCU and backend, eraseInstruction. However, in the future we will make the RCU the owner of instructions, so that coupling between components will disappear, as the RCU will be responsible for eraseInstrcuction.

mattd updated this revision to Diff 145424.May 6 2018, 8:45 PM

Clean up the in the lambda in Scheduler::select by joining the queries for an instruction and its description.

mattd marked an inline comment as done.May 6 2018, 8:46 PM
andreadb accepted this revision.May 7 2018, 3:29 AM

A few minor nits. Otherwise LGTM.

Thanks!

tools/llvm-mca/HWEventListener.h
52

Can you use a different name for the InstRef argument and the class field. Using the same name for both makes me feel a bit uncomfortable.

tools/llvm-mca/Instruction.h
20

I still think you should remove this include. It is only there because of method print, which should be also guarded against NDEBUG.

51

I think this should be guarded against NDEBUG.

tools/llvm-mca/LSUnit.h
19

I think you can just forward declare InstRef inside namespace mca. You probably don't need to include the entire "Instruction.h". Uses of InstRef are all in the cpp file (that file includes already Instruction.h).

This revision is now accepted and ready to land.May 7 2018, 3:29 AM
mattd updated this revision to Diff 145496.May 7 2018, 10:19 AM
mattd marked 4 inline comments as done and an inline comment as not done.
  • Instruction.h: NDEBUG guarded the InstRef::print()
  • HWEventListener.h: Renamed a formal so that it is does not have the same name as a member variable.
  • LSUnit.h: Forward decl 'class InstRef' and remove the inclusion of Instruction.h
mattd added a comment.May 7 2018, 10:20 AM

Thanks for the review!

tools/llvm-mca/Instruction.h
20

I get build errors regarding raw_stream if the header is not there for debug builds, so I think it needs to stay, at least to support the overloaded output operator. I could guard the include directive; however, I don't see anywhere in the compiler that specifically guards against that header.

This revision was automatically updated to reflect the committed changes.