[RyuJit/ARMARCH] lower arg with list of float fields. #14753

sandreenko · 2017-10-30T22:22:25Z

Support transformation of Hfa that is represented as field list of floats to field list of integers.

It is a temporary solution before we get rid of gtRegNum assignments in the lowering.

Signed-off-by: Sergey Andreenko <seandree@microsoft.com>

Copy was raplced with bitcast many commits ago.

sandreenko · 2017-10-30T22:36:50Z

Fix #14549 and #14377

sandreenko · 2017-10-31T16:04:07Z

@dotnet-bot
test Windows_NT x64_arm64_altjit Checked Build and Test
test Windows_NT x86_arm_altjit Checked Build and Test
test Windows_NT x86_arm_altjit Checked jitstress1
test Windows_NT x86_arm_altjit Checked tailcallstress
test Windows_NT x64_arm64_altjit Checked jitstress1
test Windows_NT x64_arm64_altjit Checked tailcallstress

sandreenko · 2017-10-31T18:05:25Z

Ok, Windows_NT x86_arm_altjit Checked tailcallstress shows that SIMD tests passed (Test Result (38 failures / -11)),
A regression in Windows_NT x64_arm64_altjit Checked jitstress1 is unrelated (unstable test).

sandreenko · 2017-10-31T18:06:16Z

@dotnet/arm32-contrib PTAL.

CarolEidt · 2017-10-31T18:50:20Z

src/jit/lower.cpp

+        assert(argNum == 0);
+
+        unsigned fieldNum = 0;
+        for (GenTreeFieldList *list = arg->AsFieldList(); list != nullptr; list = list->Rest(), fieldNum++)


I don't think this is correct for non-HFA struct types. I think that the caller of this method should always iterate through a FIELD_LIST, and then call LowerFloatArg on its list items that require it. I think that will also be cleaner.

I do not understand,

the caller of this method should always iterate through a FIELD_LIST

we do not always have a FIELD_LIST . Do you want to move this if-condition to the LowerArg? Could you please give an example when this code is incorrect?

What I meant was that if the arg is a FIELD_LIST, then you need to iterate through all of its members. Some may be floating point, and some may not.

Got it, thank you. If it is possible than we have a FIELD_LIST with different types in it, which type is set on the FIELD_LIST? And how is register information encoded in info->regnum?

Each FIELD_LIST should only have the type of the field. The presence of a FIELD_LIST node is an indication that the argument is an aggregate (either a struct or a decomposed long). When it is passed in register(s) the PUTARG node will be a multireg node that has the register list.

struct fgArgTabEntry:: regNumber regNum; // The (first) register to use when passing this argument, set to REG_STK for arguments passed on the stack.

Because regnum is just an int, we can't encode that the first field list member is passed through int register, the second float is passed through float register etc. There is no enough information to construct PUTARG.

Hmm - then perhaps I misremember. It must be the case that on arm only HFAs are passed in registers (not other struct types). I still think that it would be better and cleaner to iterate through the FIELD_LIST in the caller to this method, and not have it call itself on the child of the FIELD_LIST node.

On ARM, only HFA are passed in floating-point registers. All structs can be passed, or partially passed, in integer registers.

All structs can be passed, or partially passed, in integer registers.

So does that mean that if we have a TYP_DOUBLE field, if that struct is promoted, we could expect to encounter the TYP_DOUBLE in a FIELD_LIST under a PUTARG_REG or PUTARG_SPLIT?

CarolEidt · 2017-10-31T18:50:46Z

src/jit/lower.cpp

+//    return arg if there was in place transformation;
+//    return a new tree if the root was changed.
+//
+GenTreePtr Lowering::LowerFloatArg(GenTreePtr arg, fgArgTabEntryPtr info, unsigned argNum)


I believe it is the case that we are moving away from GenTreePtr to GenTree*.

I have not heard about it, do we?

This has come up a few times. Here's a relatively recent reference: #14020 (comment)

We should probably put it in the coding conventions. @BruceForstall what do you think?

I agree that it's weird that GenTreePtr is an oddball. Maybe if we want to stop using it, we should agree and then do a copy/replace everywhere. Generally, though, I'm ambivalent about it.

Will change it before the merge, do not want to rerun tests now.

CarolEidt · 2017-10-31T19:00:03Z

src/jit/lower.cpp

+            GenTreePtr intNode = LowerFloatArg(node, info, fieldNum);
+            if (intNode != nullptr)
+            {
+                ReplaceArgWithPutArgOrBitcast(list->pCurrent(), intNode);


A FIELD_LIST doesn't always pass its fields in registers. This should check, because we shouldn't generate a BITCAST for a stack-passed field.

Yes, you are right, this check is right below, line 1416. If it is stack location, then the function returns nullptr.

sandreenko · 2017-11-01T00:10:33Z

@dotnet-bot
test Windows_NT x64_arm64_altjit Checked Build and Test
test Windows_NT x86_arm_altjit Checked Build and Test
test Windows_NT x86_arm_altjit Checked jitstress1
test Windows_NT x86_arm_altjit Checked tailcallstress
test Windows_NT x64_arm64_altjit Checked jitstress1
test Windows_NT x64_arm64_altjit Checked tailcallstress

sandreenko · 2017-11-01T05:58:39Z

PR was updated, I have tried to make the new functions clearer.

sandreenko · 2017-11-01T17:31:26Z

@dotnet-bot
test Ubuntu x64 Innerloop Formatting
test Windows_NT x64 Innerloop Formatting

CarolEidt

LGTM - thanks!

sandreenko · 2017-11-01T19:06:55Z

Merged, thanks for the review.

Sergey Andreenko added 2 commits October 30, 2017 10:46

apply bitcast on reg fields

7907a97

Signed-off-by: Sergey Andreenko <seandree@microsoft.com>

rename ReplaceArgWithPutArgOrBitcast

6a6627e

Copy was raplced with bitcast many commits ago.

dnfclas added the cla-already-signed label Oct 30, 2017

sandreenko added arch-arm32 area-CodeGen labels Oct 30, 2017

sandreenko changed the title ~~[RyuJit/ARMARCH] lower arg with list of float fields.~~ [WIP][RyuJit/ARMARCH] lower arg with list of float fields. Oct 31, 2017

add comments

d073595

sandreenko force-pushed the bitcastForFields branch from 114fe31 to d073595 Compare October 31, 2017 07:21

sandreenko changed the title ~~[WIP][RyuJit/ARMARCH] lower arg with list of float fields.~~ [RyuJit/ARMARCH] lower arg with list of float fields. Oct 31, 2017

sandreenko requested review from BruceForstall and CarolEidt October 31, 2017 18:14

CarolEidt suggested changes Oct 31, 2017

View reviewed changes

Sergey Andreenko added 2 commits October 31, 2017 16:05

delete GenTreePtr from the new function.

05037a1

refactoring

d62e3e6

CarolEidt approved these changes Nov 1, 2017

View reviewed changes

sandreenko merged commit da2f7df into dotnet:master Nov 1, 2017

sandreenko deleted the bitcastForFields branch November 1, 2017 19:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RyuJit/ARMARCH] lower arg with list of float fields. #14753

[RyuJit/ARMARCH] lower arg with list of float fields. #14753

sandreenko commented Oct 30, 2017 •

edited

Loading

sandreenko commented Oct 30, 2017

sandreenko commented Oct 31, 2017

sandreenko commented Oct 31, 2017

sandreenko commented Oct 31, 2017

CarolEidt Oct 31, 2017

sandreenko Oct 31, 2017

CarolEidt Oct 31, 2017

sandreenko Oct 31, 2017

CarolEidt Oct 31, 2017

sandreenko Oct 31, 2017 •

edited

Loading

CarolEidt Oct 31, 2017

BruceForstall Oct 31, 2017

CarolEidt Oct 31, 2017

CarolEidt Oct 31, 2017

sandreenko Oct 31, 2017

CarolEidt Oct 31, 2017

BruceForstall Oct 31, 2017

sandreenko Oct 31, 2017

CarolEidt Oct 31, 2017

sandreenko Oct 31, 2017

sandreenko commented Nov 1, 2017

sandreenko commented Nov 1, 2017 •

edited

Loading

sandreenko commented Nov 1, 2017

CarolEidt left a comment

sandreenko commented Nov 1, 2017

[RyuJit/ARMARCH] lower arg with list of float fields. #14753

[RyuJit/ARMARCH] lower arg with list of float fields. #14753

Conversation

sandreenko commented Oct 30, 2017 • edited Loading

sandreenko commented Oct 30, 2017

sandreenko commented Oct 31, 2017

sandreenko commented Oct 31, 2017

sandreenko commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandreenko Oct 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandreenko commented Nov 1, 2017

sandreenko commented Nov 1, 2017 • edited Loading

sandreenko commented Nov 1, 2017

CarolEidt left a comment

Choose a reason for hiding this comment

sandreenko commented Nov 1, 2017

sandreenko commented Oct 30, 2017 •

edited

Loading

sandreenko Oct 31, 2017 •

edited

Loading

sandreenko commented Nov 1, 2017 •

edited

Loading