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

JIT: Make BasicBlock jump target private #93152

Merged
merged 4 commits into from
Oct 10, 2023

Conversation

amanasifkhalid
Copy link
Member

Next step for #93020. CC @dotnet/jit-contrib.

@ghost ghost assigned amanasifkhalid Oct 6, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Oct 6, 2023
@ghost
Copy link

ghost commented Oct 6, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Next step for #93020. CC @dotnet/jit-contrib.

Author: amanasifkhalid
Assignees: amanasifkhalid
Labels:

area-CodeGen-coreclr

Milestone: -

@BruceForstall BruceForstall self-requested a review October 6, 2023 20:10

BasicBlock* GetJumpDest() const
{
return bbJumpDest;
Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to take these new methods as an opportunity to assert we are accessing the jump target correctly. For example, we should only read bbJumpDest if bbJumpKind is valid, and bbJumpDest should not be null for such blocks (and if we can assert bbJumpDest is only accessed when bbJumpKind is valid, then perhaps there's no need to set bbJumpDest to null when setting bbJumpKind to something like BBJ_NONE). However, these asserts turned out to be overly aggressive -- we seem to have some edge cases in a couple of phases that break these assumptions. For one, we do set and compare bbJumpDest to null in a few places, and checking if the jump kind is valid instead of null-checking bbJumpDest would probably hurt TP (as we would have to compare bbJumpKind to multiple possible valid values, versus just one null check). Would it be worth trying to strengthen our bbJumpKind/bbJumpDest abstraction if it comes at the cost of TP?


BasicBlock* GetJumpDest() const
{
return bbJumpDest;
Copy link
Member

Choose a reason for hiding this comment

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

As a next step, this could have an assert for those kinds where bbJumpDest is legal:

assert(KindIs(BBJ_CALLFINALLY,BBJ_ALWAYS,BBJ_EHCATCHRET,BBJ_LEAVE,BBJ_COND));

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to include that too, but this proved to be overly aggressive in a few spots. For example, in Compiler::fgFindBlockILOffset(), we check curr's bbJumpDest as long as it's not a switch statement. I could check if curr's jump kind is valid first to satisfy the assert in GetJumpDest(), but I'm not sure what our stance is on reducing TP to satisfy this constraint.


void SetJumpDest(BasicBlock* jumpDest)
{
bbJumpDest = jumpDest;
Copy link
Member

Choose a reason for hiding this comment

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

maybe the same assert here, which would assume the bbJumpKind is set before changing/setting the bbJumpDest.


bool JumpsTo(const BasicBlock* jumpDest) const
{
return (bbJumpDest == jumpDest);
Copy link
Member

Choose a reason for hiding this comment

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

Same assert here.

I wonder if we should treat switch and non-switch more uniformly. i.e., JumpsTo might imply any flow graph control transfer, including switches. Maybe we'll evolve this over time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Asserting bbJumpKind is valid proved to be too aggressive in JumpsTo(), as well. Should I try enabling these asserts in a follow-up PR? I think that refactor will be nontrivial, since we null-check bbJumpDest for non-jump blocks in a bunch of places.

Copy link
Member

Choose a reason for hiding this comment

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

A follow-up PR to add asserts is fine.

Copy link
Member

Choose a reason for hiding this comment

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

It seems odd that SetJumpKind has a Compiler* arg but SetJumpKindAndTarget doesn't. I would expect the latter to be a logical superset.

If you added a Compiler* arg you could then fix the switch version to allocate the BBswtDesc automatically, perhaps.


void SetJumpSwt(BBswtDesc* jumpSwt)
{
bbJumpSwt = jumpSwt;
Copy link
Member

Choose a reason for hiding this comment

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

Also assert(KindIs(BBJ_SWITCH)); here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about this, but we have several instances where we set bbJumpSwt before updating bbJumpKind, and I wouldn't want to enforce an order of the function calls -- though maybe that means we should have a function for converting the block to BBJ_SWITCH that updates bbJumpKind and bbJumpSwt simultaneously?

Copy link
Member

Choose a reason for hiding this comment

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

we should have a function for converting the block to BBJ_SWITCH that updates bbJumpKind and bbJumpSwt simultaneously?

Maybe an overloaded:

SetKindAndTarget(BBjumpKinds kind, BasicBlock* target) => set bbJumpKind/bbJumpDest
SetKindAndTarget(BBjumpKinds kind, BBswtDesc* target) => set bbJumpKind/bbJumpSwt

for those cases where both are being set?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure thing, I'll add that in.

/* The following union describes the jump target(s) of this block */
union {
unsigned bbJumpOffs; // PC offset (temporary only)
BasicBlock* bbJumpDest; // basic block
Copy link
Member

Choose a reason for hiding this comment

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

You could imagine that BBJ_COND could have, here, struct { BasicBlock* bbJumpTrue; BasicBlock* bbJumpFalse; } where the true case would currently map to bbJumpDest and the false case to bbNext. And have accessors for each. Maybe that's where this will evolve?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that's Andy's plan. He mentions in #93020 adding an explicit fall-through successor pointer to BasicBlock that would apply to BBJ_ALWAYS, BBJ_COND, etc. Would it be worth including that work in this PR, or save it for the next one? That change will probably introduce some TP diffs...

Copy link
Member

Choose a reason for hiding this comment

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

Let's hold off... where I'd really like to get to is that these references are either:

  • some kind of BasicBlockReference holder struct (so reassignment updates ref counts automagically); or even better, perhaps:
  • FlowEdge(s)

The appeal of BasicBlockReference is that someday perhaps I'd like to be able to enumerate all the things that refer a block, including "external" BasicBlock references as well (references from the EH table, from SSA defs, special block references from the compiler object (fgFirstBB, etc), etc).

We could then eliminate all the fussing about with bbRefCounts and either have them maintained automatically or recomputed when needed.

The appeal of FlowEdges is that it simplifies some of the forward likelihood analysis, currently to find an edge to a successor you need to find the successor block and then enumerate the predecessor edges.

But not sure I want to do any of this just yet.

Copy link
Contributor

@SingleAccretion SingleAccretion Oct 6, 2023

Choose a reason for hiding this comment

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

While we are discussing successor edges, I'd like to bring up the fact there is duplication between IR control flow nodes such as SWITCH/JTRUE and the block kinds. In some systems, information about successors is determined purely from the IR and "terminator" nodes.

It is an appealing system from the orthogonality/elegance perspective since there is no duplication, but it is unlikely to be suitable for us due to the throughput cost (of at least two additional indirections in HIR).

@@ -5260,7 +5260,7 @@ class AssertionPropFlowCallback
{
ASSERT_TP pAssertionOut;

if (predBlock->KindIs(BBJ_COND) && (predBlock->bbJumpDest == block))
if (predBlock->KindIs(BBJ_COND) && predBlock->JumpsTo(block))
Copy link
Contributor

@Wraith2 Wraith2 Oct 6, 2023

Choose a reason for hiding this comment

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

At first glace i wasn't certain what JumpsTo(block) was doing, it could be getting or setting, reading the original made it clear. Consider using IsJumpTo which would make it clearer.

Copy link
Member

Choose a reason for hiding this comment

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

I would object to IsJumpsTo() -- that just doesn't read well. If consensus is that JumpsTo() is confusing, I'd recommend leaving these cases as block->JumpTarget() == block2

Copy link
Member

Choose a reason for hiding this comment

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

I would actually prefer something like HasJumpTo since JumpsTo to me reads like there's no other option (that is, BBJ_ALWAYS) and the "has" makes it clearer this is a predicate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Either HasJumpTo or JumpTarget() == block2 sound ok to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed JumpsTo to HasJumpTo, and kept JumpsToNext the same since it indicates whether a block will always "jump" to bbNext regardless of its condition.

@amanasifkhalid
Copy link
Member Author

So asserting KindIs(BBJ_SWITCH) in BasicBlock::GetJumpSwt() seems to be overly aggressive, too. This assert hits while building SPC.dll.

@AndyAyersMS
Copy link
Member

I like to see a PR with these sorts of mechanical changes without changing behavior, and then see behavioral changes as separate follow-ups. There are so many diffs here with renaming that it is easy to miss something that changes things more fundamentally.

In practice you may find you need to do both at the same time to figure out which mechanical changes you want, but you can either keep separate commits locally and PR them one by one, or (more painful) split a commit up to defer the behavior changing parts.

@amanasifkhalid
Copy link
Member Author

@AndyAyersMS @BruceForstall PTAL. Per Bruce's suggestion, I added a helper method for simultaneously updating the jump kind and target, and asserting that the jump kind argument is valid (BBJ_ALWAYS, BBJ_COND, etc) doesn't seem to break any of our current invariants.

Since we probably want to ensure bbJumpDest is read only when a block contains a jump, I'd like your opinion about how to handle the instances where we currently set/compare bbJumpDest to null. For example, when we first initialize a block, we set its jump kind in bbNewBasicBlock, but not its jump target, so it is possible for a block with a jump to have a null jump target for a period of time after its creation. Also, when converting a block's jump kind to BBJ_NONE, we sometimes set its jump target to null, and other times leave it be under the assumption we won't be reading it. Then, there are plenty of asserts where we compare the block's jump target to null -- we either assert a block with a valid jump kind doesn't have a null target, or a block with an invalid jump kind does have a null target. And as a quick way to check if a block has a jump, we sometimes check if its jump target is not null in conditions.

I imagine checking if the jump target is null is cheaper (in terms of TP) than checking if the jump kind is valid, since we'd have to check against multiple jump kinds each time. So maybe it's beneficial to allow bbJumpDest to be nullable, but then we cannot ensure bbJumpDest is read only for blocks with valid jump kinds, which limits our ability to do the asserts Bruce suggested above. Maybe we could standardize setting bbJumpDest to null when setting bbJumpKind to a non-jump kind, and then add a helper method like HasJump() that simply checks if bbJumpDest is null (and asserts bbJumpKind is consistent with whether bbJumpDest is null or not)? This would replace calls like HasJumpTo(nullptr) and remove calls like SetJumpDest(nullptr), allowing us to assert those methods are called only for blocks with valid jump kinds. As for new blocks with a valid jump kind and no jump target yet, I don't think we are reading their (null) jump target until it is set anyway, so we can similarly assert that GetJumpDest() is only called on blocks with valid jump kinds, and the jump target is never null.

@BruceForstall
Copy link
Member

Since bbJumpDest is part of a union, the "tag" or type of which is the bbJumpKind, it generally seems semantically illegal to access bbJumpDest without first checking bbJumpKind. This is especially true if the union expands in the future. Structurally that isn't true, because checking bbJumpDest != nullptr is the same as checking bbJumpSwt != nullptr. It would be nice to "hide" that in a BasicBlock predicate function, so we can get the same throughput as today with better encapsulation of that detail.

In general, it makes sense to always null out bbJumpDest if the bbJumpKind doesn't use it, and require it be non-null if bbJumpKind requires it. It makes for a simple, easy-to-understand invariant. During block creation, it might require some refactoring to achieve that, perhaps by deferring setting the kind until the dest is known?

I dislike HasJumpTo(nullptr) (maybe HasJumpTo(block) should assert(block != nullptr)), so having a HasJump() instead makes sense.

@amanasifkhalid
Copy link
Member Author

During block creation, it might require some refactoring to achieve that, perhaps by deferring setting the kind until the dest is known?

I dislike HasJumpTo(nullptr) (maybe HasJumpTo(block) should assert(block != nullptr)), so having a HasJump() instead makes sense.

The bbNewBasicBlock refactor sounds like a good idea. I'll include that and the HasJump() work in a follow-up PR.

@amanasifkhalid
Copy link
Member Author

FYI: small TP diffs.

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Mostly looks good...


bool JumpsTo(const BasicBlock* jumpDest) const
{
return (bbJumpDest == jumpDest);
Copy link
Member

Choose a reason for hiding this comment

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

It seems odd that SetJumpKind has a Compiler* arg but SetJumpKindAndTarget doesn't. I would expect the latter to be a logical superset.

If you added a Compiler* arg you could then fix the switch version to allocate the BBswtDesc automatically, perhaps.

@amanasifkhalid
Copy link
Member Author

@AndyAyersMS I initially intended for us to use SetJumpKindAndTarget only for blocks that have a jump, so we wouldn't have a scenario where we need the Compiler* arg to assert BBJ_NONE is being used correctly. But if you'd prefer, I can adjust this implementation so that we use SetJumpKindAndTarget for all blocks regardless of jump type (where the jumpDest arg is null for blocks without jumps), and add a Compiler* arg to it so we can do the BBJ_NONE-related assertions. This approach would help us enforce bbJumpDest is null for blocks without jumps, plus we can assert SetJumpKind and SetJumpDest are used only for blocks that have jumps so we don't break the bbJumpDest == nullptr invariant for blocks without jumps.

Should I include that change here or in the follow-up PR?

@AndyAyersMS
Copy link
Member

Should I include that change here or in the follow-up PR?

Follow up is fine, just trying to understand where things are headed.

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Changes LGTM.

@amanasifkhalid amanasifkhalid merged commit e14540a into dotnet:main Oct 10, 2023
129 checks passed
amanasifkhalid added a commit that referenced this pull request Oct 18, 2023
Followup to #93152. This refactor enforces new invariants on BasicBlock's bbJumpKind and bbJumpDest. In particular, whenever bbJumpKind is a kind that must have a jump target, bbJumpDest must be set, else bbJumpDest must be null. This means bbJumpKind and bbJumpDest must be simultaneously initialized/updated when creating/converting a jump block.
@ghost ghost locked as resolved and limited conversation to collaborators Nov 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants