-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Arm64: Pass promoted struct using GT_FIELD_LIST #17911
Conversation
Fixes #16377 |
@sdmaclea @dotnet/jit-contrib PTAL |
ping @dotnet/jit-contrib (I misspelled jit-contrib in the previous comment) |
src/jit/morph.cpp
Outdated
} | ||
if (lcl != nullptr) | ||
{ | ||
// Its fields will need to accessed by address. | ||
lvaSetVarDoNotEnregister(lcl->gtLclNum DEBUG_ARG(DNER_IsStructArg)); | ||
unsigned lclNum = lcl->gtLclNum; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This local variable doesn't seem necessary
src/jit/morph.cpp
Outdated
LclVarDsc* varDsc = &(lvaTable[lcl->gtLclNum]); | ||
assert(varDsc->lvPromoted == true); | ||
|
||
unsigned lclNum = lcl->gtLclNum; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused?
src/jit/morph.cpp
Outdated
arg = gtNewOperNode(GT_ADDR, TYP_I_IMPL, arg); | ||
arg = gtNewObjNode(lvaGetStruct(lcl->gtLclNum), arg); | ||
} | ||
// Its fields will need to accessed by address. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be accessed?
test Ubuntu arm64 Cross Checked Innerloop Build and Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@CarolEidt Thanks |
test Ubuntu arm64 Cross Checked Innerloop Build and Test |
@CarolEidt It didn't trigger for me either. It might not exist yet or I spelled it wrong... |
@dotnet-bot help |
Welcome to the dotnet/coreclr Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
Welcome to the dotnet/coreclr Perf help The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
Welcome to the dotnet/coreclr Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
Hmmm - seems to have magically triggered now. Perhaps one just needs to ask for help ;-) |
@CarolEidt can you add the regression test to the arm64 lst file? I opened a pr against this pr which I believe should be sufficient see CarolEidt#1 for more info. |
@@ -0,0 +1,97 @@ | |||
using System; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need the standard header above.
test Ubuntu arm64 Cross Checked Innerloop Build and Test |
test Windows_NT x64 Formatting |
@dotnet-bot test Windows_NT arm64 Cross Checked normal Build and Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks Good
tests/arm64/Tests.lst
Outdated
@@ -90787,3 +90787,11 @@ Expected=0 | |||
MaxAllowedDurationSeconds=600 | |||
Categories=EXPECTED_PASS | |||
HostStyle=0 | |||
|
|||
[GitHub_16377.cmd_11720] | |||
RelativePath=JIT\Regression\JitBlue\GitHub_16377\GitHub_16377.cmd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this should be JIT\Regression\JitBlue\GitHub_16377\GitHub_16377\GitHub_16377.cmd sorry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CarolEidt#2 to address it.
@dotnet/dnceng - is there a known problem with the CI system? Jobs aren't triggering, and I've had a couple of process-seeming failures. |
@CarolEidt can you clarify which runs are missing here? (the non-finished ones in this PR are all in progress). There have been some issues around machine provisioning this week but I need some more specifics to be helpful. |
@MattGal - after the latest push, only 7 jobs seem to have triggered. The arm64 jobs, for example, didn't pass in the last push, and need to re-run. But normally they would get re-triggered with a new commit. |
Got it, created https://github.com/dotnet/core-eng/issues/3447 to investigate. |
test this please |
@dotnet-bot test this please |
@dotnet-bot test Windows_NT arm64 Cross Checked normal Build and Test |
@dotnet-bot test Ubuntu arm64 Cross Checked Innerloop Build and Test |
Add header to test case.
@dotnet-bot test Ubuntu arm64 Cross Checked Innerloop Build and Test |
@CarolEidt the ubuntu arm64 job is not worth running. We have a build only currently to give some support; however, we do not have the hardware to run the checked jobs yet. |
@jashook - thanks. Would you have any additional arm64 legs that you would suggest I run before merging this? |
@dotnet-bot test Windows_NT arm64 Cross Checked normal Build and Test, to run the pri 1 tests I think is sufficient, unless you believe this requires stress :) |
I'm not sure that stress testing would check anything really useful for struct passing. @sdmaclea is there anything additional you think I should run? |
You could run corefx tests, but I do not think those are enabled for arm64 yet. #17764 is trying to enable them. If you want me to pull your PR and run corefx, let me know, but it looked like your test cases should cover everything. |
FYI - this results in the following diffs, using protononjit:
The majority of the methods are in the X86 HardwareIntrinsics tests, where, instead of |
No description provided.