Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend stackmap format to allow for derived types. #40

Merged
merged 1 commit into from
Aug 18, 2022

Conversation

vext01
Copy link

@vext01 vext01 commented Aug 17, 2022

Prior to this change, LLVM would write bogus stackmap entries for
larger derived types:
llvm#55957

To record the correct information in the stackmap section, we need to
make two changes:

  1. The ability for the stackmap format to express that any value could
    have been split among multiple register and/or memory locations. The
    stackmap format cannot, at-present, express this.

    Whereas before the entity relationship diagram was:

    (stackmap-callsite) -has-many-> (location)

    Now it is:

    (stackmap-callsite) -has-many-> (live-variable) -has-many-> (location)

  2. The ability for the relevant DAG nodes to encode live variable
    groupings.

    This applies to stackmap, patchpoint and statepoint DAG nodes (which
    are all intertwined, otherwise I'd have left statepoint alone).

    Before, the relevant node operands were a flat list, and could not
    express that values could be made up of many other values. This change
    introduces a NextLive marker to encode how operands are grouped into
    live variables.

    For example, live variable operands in a DAG node like this:

    [1, 2, NextLive, 3, NextLive, 4, 5, 6, NextLive]

    expresses three live variables, made of:

    1. [1, 2]
    2. [3]
    3. [4, 5, 6]

Unfortunately changing the DAG nodes caused a lot of test failures. I've
fixed most of them, but since we don't plan to upstream this right now,
I've skipped some of the tests that are very annoying to update.

Currently structs and vectors (aggregate types) are now correctly
recorded in the stackmap section

Vectors don't yet work and will require more work (they use a different
DAG node structure which will require walking the graph to infer how to
encode the stackmap operands). That work will build on this change.

A companion PR will follow: ykjit/yk#561

while (ExpectArgs) {
SDNode *ItN = It->getNode();
if ((ItN->getOpcode() == ISD::TargetConstant) &&
(cast<ConstantSDNode>(ItN)->getZExtValue() == StackMaps::NextLive))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I misread this one at first: it really feels like {...} brackets would help here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 362242d

@vext01
Copy link
Author

vext01 commented Aug 18, 2022

OK to squash?

std::stringstream Msg;
Msg << "stackmap ID " << CSI.ID
<< ": too many locations in live variable #" << LiveIdx;
OS.getContext().reportError(SMLoc(), Msg.str());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we return at this point or ... ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we should. It prevents code from crashing later and spamming the console with a backtrace! Good catch.

Fixed in 0896c8ff40bf

@ltratt
Copy link

ltratt commented Aug 18, 2022

Please squash.

Prior to this change, LLVM would write bogus stackmap entries for
larger derived types:
llvm#55957

To record the correct information in the stackmap section, we need to
make two changes:

1) The ability for the stackmap format to express that any value could
   have been split among multiple register and/or memory locations. The
   stackmap format cannot, at-present, express this.

   Whereas before the entity relationship diagram was:

     (stackmap-callsite) -has-many-> (location)

   Now it is:

     (stackmap-callsite) -has-many-> (live-variable) -has-many-> (location)

2) The ability for the relevant DAG nodes to encode live variable
   groupings.

   This applies to stackmap, patchpoint and statepoint DAG nodes (which
   are all intertwined, otherwise I'd have left statepoint alone).

   Before, the relevant node operands were a flat list, and could not
   express that values could be made up of many other values. This change
   introduces a `NextLive` marker to encode how operands are grouped into
   live variables.

   For example, live variable operands in a DAG node like this:

     [1, 2, NextLive, 3, NextLive, 4, 5, 6, NextLive]

   expresses three live variables, made of:
    1) [1, 2]
    2) [3]
    3) [4, 5, 6]

Unfortunately changing the DAG nodes caused a lot of test failures. I've
fixed most of them, but since we don't plan to upstream this right now,
I've skipped some of the tests that are very annoying to update.

Currently structs and vectors (aggregate types) are now correctly
recorded in the stackmap section

Vectors don't yet work and will require more work (they use a different
DAG node structure which will require walking the graph to infer how to
encode the stackmap operands). That work will build on this change.
@vext01
Copy link
Author

vext01 commented Aug 18, 2022

splat.

@ltratt
Copy link

ltratt commented Aug 18, 2022

bors r+

@bors
Copy link

bors bot commented Aug 18, 2022

Build succeeded:

@bors bors bot merged commit 22e35b8 into ykjit:main Aug 18, 2022
@vext01 vext01 deleted the sm-aggregates branch August 19, 2022 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants