Skip to content

Conversation

@SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Aug 24, 2022

Three commits:

  1. No need to explicitly simplify OBJ(ADDR(LCL_VAR)) anymore. No diffs.
  2. No need to explicitly fold things anymore. A couple small regressions in exotic test code.
  3. Dead code removal. No diffs.

Diffs.

@ghost ghost added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member labels Aug 24, 2022
@ghost
Copy link

ghost commented Aug 24, 2022

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

Issue Details

Three commits:

  1. No need to explicitly simplify OBJ(ADDR(LCL_VAR)) anymore. No diffs.
  2. No need to explicitly fold things anymore. A couple small regressions in exotic test code.
  3. Dead code removal. No diffs.
Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

This was originally put in place to allow the corresponding codegen
changes to be tested. Now that we have more than adequate coverage
from the front-end transformations, we can simply delete the code.
We don't expect any OBJs here.
@SingleAccretion SingleAccretion marked this pull request as ready for review August 25, 2022 14:03
@SingleAccretion
Copy link
Contributor Author

@dotnet/jit-contrib

@JulieLeeMSFT JulieLeeMSFT added this to the 8.0.0 milestone Aug 25, 2022
@AndyAyersMS
Copy link
Member

@kunalspathak can you review?

@kunalspathak
Copy link
Contributor

No need to explicitly simplify OBJ(ADDR(LCL_VAR)) anymore. No diffs.

I am not much familiar with this code lately. Can you elaborate in PR comments why explicit simplification is not needed now?

@SingleAccretion
Copy link
Contributor Author

Why is explicit simplification is not needed now?

Essentially, we are now folding this pattern in local morph, and so don't need the additional checks. Eventually, almost all such checks will be deleted from the codebase; this is a start.

{
if (m_dst->OperIs(GT_OBJ))
{
GenTreeLclVar* lclVar = m_comp->fgMorphTryFoldObjAsLclVar(m_dst->AsObj());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed after #71584.


// First, handle the GT_OBJ case, which loads into the arg register
// (so we don't set the use to prefer that register for the source address).
if (op1->OperIs(GT_OBJ))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dead code.

#endif // !TARGET_X86
}

if (src->OperIs(GT_OBJ) && src->AsObj()->Addr()->OperIsLocalAddr())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed after #70960 and #70779.

// STRUCT args (FIELD_LIST / OBJ / LCL_VAR / LCL_FLD) will always be contained.
MakeSrcContained(putArgNode, src);

// TODO-ADDR: always perform this transformation in local morph and delete this code.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed after #71598 and #71328.

@kunalspathak
Copy link
Contributor

Great...thank you. I am not able to access the diff...what does it look like?

@SingleAccretion
Copy link
Contributor Author

what does it look like?

There are no diffs outside of coreclr_tests. In tests, we have:

  1. On all non-ARM platforms: 4 minor regressions in tests that test Call(*(Struct*)&doubleVal);-like code.
  2. On ARM: couple large improvements because some huge structs now go down the OBJ(LCL_FLD_ADDR) path, which is more optimal for them (reversal of Support PUTARG_SPLIT(STRUCT LCL_VAR/LCL_FLD) on ARM/64 #70861 (comment), essentially).

Copy link
Contributor

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM

@kunalspathak kunalspathak merged commit 2500b6e into dotnet:main Sep 13, 2022
@SingleAccretion SingleAccretion deleted the No-Obj-Folding branch September 13, 2022 18:21
@ghost ghost locked as resolved and limited conversation to collaborators Oct 13, 2022
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 community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants