Page MenuHomePhabricator

[SDAG] Remove the reliance on MI's allocation strategy for `MachineMemOperand` pointers attached to `MachineSDNodes` and instead have the `SelectionDAG` fully manage the memory for this array.

Authored by chandlerc on Aug 13 2018, 4:47 PM.



Prior to this change, the memory management was deeply confusing here --
The way the MI was built relied on the SelectionDAG allocating memory
for these arrays of pointers using the MachineFunction's allocator so
that the raw pointer to the array could be blindly copied into an
eventual MachineInstr. This creates a hard coupling between how
MachineInstrs allocate their array of MachineMemOperand pointers and
how the MachineSDNode does.

This change is motivated in large part by a change I am making to how
MachineFunction allocates these pointers, but it seems like a layering
improvement as well.

This would run the risk of increasing allocations overall, but I've
implemented an optimization that should avoid that by storing a single
MachineMemOperand pointer directly instead of allocating anything.
This is expected to be a net win because the vast majority of uses of
these only need a single pointer.

As a side-effect, this makes the API for updating a MachineSDNode and
a MachineInstr reasonably different which seems nice to avoid
unexpected coupling of these two layers. We can map between them, but we
shouldn't be *surprised* at where that occurs. =]

Diff Detail


Event Timeline

chandlerc created this revision.Aug 13 2018, 4:47 PM

Doh, this doesn't have the change to do the clever optimization mentioned in the change description. I'll update it with that momentarily. But that will be just a tiny implementation detail, so probably fin eto start reviewing.

chandlerc updated this revision to Diff 160495.Aug 13 2018, 5:16 PM

Update to include the optimization where we store a single MMO inline to avoid
allocation. Almost nothing changed, all tests continue to pass.

This code certainly looks cleaner.

7100 ↗(On Diff #160488)

Use std::copy?

chandlerc marked an inline comment as done.Aug 13 2018, 5:36 PM
chandlerc added inline comments.
7100 ↗(On Diff #160488)

Yep, made exactly this change when getting the right implementation here for the optimized form.

hfinkel added inline comments.Aug 13 2018, 5:40 PM
2256 ↗(On Diff #160495)

Should we use TinyPtrVector here instead of making this PointerUnion and keeping NumMemRefs below?

chandlerc marked an inline comment as done.Aug 13 2018, 5:43 PM
chandlerc added inline comments.
2256 ↗(On Diff #160495)

We can't. I tried so hard to do this. It was terrible.

In a word, MorphNodeTo.

We don't actually run constructors or destructors reliably for any SDNode subclasses. We allocate enough memory and with enough alignment for any of the subclasses, and then we just "switch" the subclass by writing to NodeType. Yay.

We have manual code in MorphNodeTo to "clean up" and "initialize" the members as needed.

I tried fixing this, and it is terrible. Nothing works.

We essentially *have* to allocate any extra storage off the DAG's allocator so it gets freed for us, and we have to use POD-ish data types here that we can update with a simple store regardless of what uninitialized garbage memory is left there due to MorphNodeTo.

Sorry I couldn't find anything less awful than this.

hfinkel added inline comments.Aug 13 2018, 6:04 PM
2256 ↗(On Diff #160495)

Makes sense. Can you please add this as a comment?

Otherwise, I'm happy with this patch.

bogner accepted this revision.Aug 13 2018, 6:18 PM

LGTM as long as Hal's good with your explanation of why we can't use TinyPtrVector.

2258–2262 ↗(On Diff #160495)

Yep, sizeof(MachineSDNode) is 96 bytes either way (it's exactly 96 before this change, and 92 + 4 bytes padding after). I don't know if we need quite this long of a comment explaining that it's fine, but I guess it doesn't hurt.

2270 ↗(On Diff #160495)

Should the doxygen of MachineSDNode mention SelectionDAG::setNodeMemRefs somewhere?

7112 ↗(On Diff #160495)

C++ style casts are arguably a bit clearer (but feel free to ignore this if you disagree)

This revision is now accepted and ready to land.Aug 13 2018, 6:18 PM
chandlerc updated this revision to Diff 160507.Aug 13 2018, 7:27 PM
chandlerc marked 6 inline comments as done.

Address review feedback.

I think all done.

2270 ↗(On Diff #160495)

Added comments explaining this.

7112 ↗(On Diff #160495)

For primitive types, it never seems worth it to me, but I don't feel strongly here. Changed.

rnk accepted this revision.Aug 14 2018, 3:57 PM
rnk added a subscriber: rnk.


5521–5522 ↗(On Diff #160507)

There is only one other call site of extractLoadMemRefs, and its only called from X86InstrInfo.cpp. Can you migrate the other call site over to your extractLoadMMOs helper and delete MachineFunction::extractLoadMemRefs? Now that we can get an ArrayRef<MachineMemOperand*> from both MachineSDNodes and MachineInstr, we should be able to share.

5631 ↗(On Diff #160507)

Ditto for stores, I think.

Thanks all. Think I have everything so landing. Give a shout if anything else jumps out.

5521–5522 ↗(On Diff #160507)

See the next patch? ;] It does exactly that.

5631 ↗(On Diff #160507)

Yep, also done in the next patch.

This revision was automatically updated to reflect the committed changes.