This is an archive of the discontinued LLVM Phabricator instance.

[RewriteStatepointsForGC] Handle extractelement fully in the base pointer algorithm
ClosedPublic

Authored by reames on Jul 24 2015, 10:57 AM.

Details

Summary

When rewriting the IR such that base pointers are available for every live pointer, we potentially need to duplicate instructions to propagate the base. The original code had only handled PHI and Select under the belief those were the only instructions which would need duplicated. When I added support for vector instructions, I'd added a collection of hacks for ExtractElement which caught most of the common cases. Of course, I then found the one test case my hacks couldn't cover. :)

This change removes all of the early hacks for extract element. By defining extractelement as a BDV (rather than trying to look through it), we can extend the rewriting algorithm to duplicate the extract as needed.

Note that I'm removing several peephole optimizations which were possible with the old scheme, but hard to express with the new one. I plan on adding these back as a separate change since it'll need to be structured rather differently. I want to get the code correct first. :)

w.r.t. testing, this is mostly covered by existing tests (live-vector.ll). I've added a couple of new ones to show the failing case (test2 in base-vector.ll). With the removal of the early peepholes, the new code should be reasonably well tested by the existing tests.

Diff Detail

Event Timeline

reames updated this revision to Diff 30585.Jul 24 2015, 10:57 AM
reames retitled this revision from to [RewriteStatepointsForGC] Handle extractelement fully in the base pointer algorithm.
reames updated this object.
reames added reviewers: sanjoy, igor-laevsky, pgavlin.
reames added a subscriber: llvm-commits.
sanjoy accepted this revision.Jul 24 2015, 12:35 PM
sanjoy edited edge metadata.

lgtm with comments inline.

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
380–382

Update this comment please.

855

Partial sentence?

857

Why is the third check isa<VectorType>(State.getBase()->getType()) needed? Can there be non-vector inputs to extractelement instructions?

888

I'm not sure using IRBuilder is appropriate if you need an Instruction for correctness -- the builder could fold an extractelement of an undef vector to an undef value.

974

As a separate change, once the dust settles, do you mind renaming these variables to LLVM style (e.g. BaseSI instead of basesel)?

1000

Should be named Base.

This revision is now accepted and ready to land.Jul 24 2015, 12:35 PM
reames updated this revision to Diff 30882.Jul 28 2015, 6:30 PM
reames edited edge metadata.

Address review comments and fix a bug I noticed. It turns out I can't yet remove the peephole optimization code without regressing existing functionality. This wasn't covered by the test case - bad Philip! - but I've added two new tests that show the issue. The short version of the problem is that we were also missing generic handling for insertelement and that the peepholes were covering that too. Gah.

reames closed this revision.Aug 12 2015, 2:01 PM

Committed revision 244808