Page MenuHomePhabricator

[RS4GC] Fix algorithm to avoid setting vector BDV for scalar derived pointer
ClosedPublic

Authored by anna on Mar 17 2020, 11:22 AM.

Details

Summary

This is a more general fix to 59029b9eef23 (D75704).
This patch does the following:

  1. updates isKnownBaseValue to account for base pointer and

derived pointer having differing types.

  1. This inturn allows us to populate the

lattice (States) for such derived pointers.

  1. It also updates all states where the base and derived pointers have

differing types (vector versus scalar) and conservatively marks these
states as conflictcs.
Note that in 59029b9eef23, we were just fixing existing lattice values
and that too, only for uses of extractelement.

Diff Detail

Event Timeline

anna created this revision.Mar 17 2020, 11:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2020, 11:22 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
anna updated this revision to Diff 250867.Mar 17 2020, 12:44 PM

fixed clang format. NFC w.r.t. prev diff.

dantrushin added inline comments.Mar 18 2020, 6:36 AM
llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
658

isa<VectorType>(V->getType()) != isa<VectorType>(Input->getType()) ?

anna updated this revision to Diff 251370.Mar 19 2020, 6:47 AM
anna marked an inline comment as done.

addressed review comments. fixed CI failure related to clang-tidy.

I hoped Philip will review this, because I'm not very familiar with this part of RS4GC and I have couple quetions here, see inline.

llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
652

Is Input a derived pointer here for which we check if V is a base of?.. Or vice versa?

895–900

Here I am confused.
Isn't Pair.second.getBaseValue() supposed to be base value for BDV? And we're asking the other way?
Same question for lines 944, 977 and few more below.

anna marked 2 inline comments as done.Mar 19 2020, 7:49 AM
anna added inline comments.
llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
652

It's actually overloaded to answer two questions:

  1. is V a knownBase?
  2. Is V and Input having differing types (Input can be either the derivedPtr where V is the BasePtr or it can be the base pointer for V)

Technically, in most cases, we don't need Input because isKnownBaseResult is just depending on whether V is a knownBase.

I think a better factoring of the code will be to have a helper function which utilizes isKnownBaseResult and *then* checks whether Base and Input are both vector or both scalar type.

That will be cleaner and avoid the confusion below.

895–900

Yes, Pair.second.getBaseValue() is supposed to be base value for BDV.
This is actually just a check for the types to make sure we don't fail the assert here.
I think a cleaner assert is this:
assert((!isKnownBaseResult(BDV) || (BDV and Pair.second.getBaseValue() have differing scalar/vector types)) && "why did it get added")

Basically, knownBases that have different scalar/vector type from the

The lines you're referring to below will also be handled in similar way.

anna planned changes to this revision.Mar 19 2020, 7:49 AM

Will avoid confusing usage with cleaner asserts.

anna updated this revision to Diff 251452.Mar 19 2020, 12:40 PM

refactored code, made asserts cleaner.

From what I see it should work. The only thing worries me is that this isCorrectType seems to be not incorporated into solution but some side hack to solve the problem.
Unfortunately at this moment I do not have a better suggestion :(

If I understand correctly this is a fix for functional bug, so probably it makes sense to land it in this form.

Let me to thing one day more, if I cannot come up with some proposal I agree on this one.

llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
667

Confusing name...
areTypesMatched? or something like this?

skatkov added inline comments.Apr 7 2020, 4:40 AM
llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
667

May be even more specific - areBothVectorOrScalar?

skatkov accepted this revision.Apr 8 2020, 4:33 AM

ok, I could not come up with something better, we can revisit at later.

Just please rename utility function to something more appropriate.

This revision is now accepted and ready to land.Apr 8 2020, 4:33 AM
anna marked 3 inline comments as done.May 14 2020, 6:17 AM
anna added inline comments.
llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
667

changed name to areBothVectorOrScalar.

anna closed this revision.May 14 2020, 7:09 AM
anna marked an inline comment as done.