[Windows|Arm64|VarArgs] Correctly pass HFA arguments by jashook · Pull Request #18364 · dotnet/coreclr

jashook · 2018-06-07T23:06:04Z

Windows arm64 VarArgs ABI requires passing using the general purpose registers. For HFA types, this would lead to bad codegen for vararg methods. Both the fixed arguments and the variable arguments are affected.

For arguments only, HFA structs need to be treated as if they are not HFAs.

briansull

Looks Good

CarolEidt · 2018-06-08T22:04:09Z

+#if !defined(_TARGET_UNIX_) && defined(_TARGET_ARM64_)
+                && !isVarArg // Arm64 Windows VarArg methods arguments will not
+                             // classify HFA types, they will need to be treated
+// as if they are not HFA types.


It would be nice to fix up this comment, if you can get the formatter to go along with a fix. Maybe put the comment above the && line.

CarolEidt · 2018-06-08T22:06:26Z

-                // Structs that are pointer sized or smaller should have been handled by getPrimitiveTypeForStruct
-                assert(structSize > TARGET_POINTER_SIZE);
+#if !defined(_TARGET_UNIX_)
+                if (!isVarArg)


I don't see why this is needed. It should still be the case that getPrimitiveTypeForStruct should return a primitive type for anything <= TARGET_POINTER_SIZE, right?

You are correct, thank you for catching this I do not know what I was thinking at the time.

CarolEidt · 2018-06-08T22:08:06Z

 //
 // Arguments:
-//    tmpVarNum  - the var num which we clone into the newly created temp var.
+//    bool isVarArg - is a VarArg method. Will effect HFA decision for arm64 windows


Nit: should be "affect"

CarolEidt · 2018-06-08T22:10:49Z


    void SortArgs();

-    void EvalArgsToTemps();


EvalArgs [](start = 9, length = 8)

Could you change this to be in the positive sense? Perhaps allowHfa?

CarolEidt · 2018-06-08T22:12:28Z

This looks good except for some minor things and one more substantive question.
I hope that eventually we can refactor both the incoming and outgoing arg handling so that we don't have to pass the hfa-related bools around all over the place.

CarolEidt

🕐

CarolEidt

LGTM - thanks! (and sorry for the delay in reviewing)

briansull

Looks Good

Previously, the type would be reported as HFA and enregistered; however, this is not correct, as arm64 varargs abi requires passing using int registers.

jashook · 2018-06-12T21:34:34Z

@CarolEidt @briansull can you take another look, I have updated and rebased against master. This pulls in #18358. I have also followed Carol's approach to using fgArgEntry to store isVararg.

This removes some of the isVararg parameters passed around.

CarolEidt

LGTM - thanks!!

CarolEidt · 2018-06-12T21:43:07Z

 //
 // Arguments:
-//    tmpVarNum  - the var num which we clone into the newly created temp var.
+//    curArgTabEntry


Thanks for catching that!

briansull

Looks Good, with a couple of comments

briansull · 2018-06-12T21:51:23Z

            gcInfo.gcMarkRegPtrVal(argReg, loadType);

-            if (compiler->lvaIsMultiregStruct(varDsc))
+            if (compiler->lvaIsMultiregStruct(varDsc, compiler->info.compIsVarArgs))


What is the effect of passing a true for compIsVarArgs as the second parameter?

If it simply makes this function always return false when varargs is true,
then it does not need to be a parameter.

Instead write:

if (!compiler->info.compIsVarArgs && compiler->lvaIsMultiregStruct(varDsc))

It will the decision for whether it is a multiregstruct for hfa cases. I believe it is necessary.

sorry i was wrong

briansull · 2018-06-12T22:11:18Z

-            if (IsHfa(clsHnd))
+#else // !ARM_SOFTFP
+            if (IsHfa(clsHnd)
+#if !defined(_TARGET_UNIX_) && defined(_TARGET_ARM64_)


It might be better to define

#if defined(_TARGET_ARM64_) && !defined(_TARGET_UNIX_) #define IGNORE_HFA_FOR_VARARG

Don't we have a TARGET_WINDOWS or WIN64?

I believe there is only a WINOWS_AMD64_ABI define.

I may be missing something.

briansull

Looks Good

jashook · 2018-06-13T22:42:15Z

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

jashook · 2018-06-13T22:44:31Z

osx timed out. Arm64 has passed. OSX has passed in last change, only difference are ifdef names. Which the build passes so I do not think it is required to wait.

…18364) * Fix passing HFA of two floats to vararg methods Previously, the type would be reported as HFA and enregistered; however, this is not correct, as arm64 varargs abi requires passing using int registers. * Address linux build issue * Apply final format patch * Add _TARGET_WINDOWS_ Commit migrated from dotnet/coreclr@c485d3f

jashook requested review from BruceForstall, CarolEidt and briansull June 7, 2018 23:06

jashook changed the title ~~[Windows][Arm64][VarArgs] Correct HFA type information for arguments~~ [Windows|Arm64|VarArgs] Correct HFA type information for arguments Jun 7, 2018

jashook changed the title ~~[Windows|Arm64|VarArgs] Correct HFA type information for arguments~~ [Windows|Arm64|VarArgs] Correctly pass HFA arguments Jun 7, 2018

jashook closed this Jun 7, 2018

jashook reopened this Jun 7, 2018

jashook force-pushed the handle_arm64_hfa_varargs branch from b481792 to 110fa8e Compare June 8, 2018 17:32

briansull approved these changes Jun 8, 2018

View reviewed changes

CarolEidt reviewed Jun 8, 2018

View reviewed changes

CarolEidt suggested changes Jun 8, 2018

View reviewed changes

jashook force-pushed the handle_arm64_hfa_varargs branch 4 times, most recently from 8cd6d87 to 359b0fd Compare June 9, 2018 01:06

CarolEidt approved these changes Jun 12, 2018

View reviewed changes

briansull approved these changes Jun 12, 2018

View reviewed changes

jashook force-pushed the handle_arm64_hfa_varargs branch from 359b0fd to 1116404 Compare June 12, 2018 21:21

Fix passing HFA of two floats to vararg methods

6439ed1

Previously, the type would be reported as HFA and enregistered; however, this is not correct, as arm64 varargs abi requires passing using int registers.

jashook force-pushed the handle_arm64_hfa_varargs branch from 1116404 to 6439ed1 Compare June 12, 2018 21:26

jashook closed this Jun 12, 2018

jashook reopened this Jun 12, 2018

CarolEidt approved these changes Jun 12, 2018

View reviewed changes

Address linux build issue

b61b800

jashook closed this Jun 12, 2018

jashook reopened this Jun 12, 2018

briansull approved these changes Jun 12, 2018

View reviewed changes

jashook added 2 commits June 13, 2018 08:51

Apply final format patch

8300490

Add _TARGET_WINDOWS_

85942e5

briansull approved these changes Jun 13, 2018

View reviewed changes

jashook merged commit c485d3f into dotnet:master Jun 13, 2018

jashook mentioned this pull request Jun 14, 2018

Update CoreClr, CoreFx, CoreSetup, ProjectNTfs, ProjectNTfsTestILC, Standard to preview1-26613-04, preview1-26613-07, preview1-26613-01, beta-26613-00, beta-26613-00, servicing-26419-02, respectively (master) dotnet/corefx#30338

Closed

Conversation

jashook commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

briansull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CarolEidt commented Jun 8, 2018

Uh oh!

CarolEidt left a comment

Choose a reason for hiding this comment

Uh oh!

CarolEidt left a comment

Choose a reason for hiding this comment

Uh oh!

briansull left a comment

Choose a reason for hiding this comment

Uh oh!

jashook commented Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CarolEidt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briansull left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briansull left a comment

Choose a reason for hiding this comment

Uh oh!

jashook commented Jun 13, 2018

Uh oh!

jashook commented Jun 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jashook commented Jun 7, 2018 •

edited

Loading

jashook commented Jun 12, 2018 •

edited

Loading

briansull left a comment •

edited

Loading