Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Conversation

wateret
Copy link
Member

@wateret wateret commented Jul 8, 2016

Compiler::argOrReturnTypeForStruct returns TYP_INT for any size-4 value type struct.
On ARM(hardfp), it is processed by HFA, however HFA is off on softfp.
So we need to check if the actual return type is TYPE_INT or TYPE_FLOAT.
Otherwise struct types with single float32 get into problems.

Check is done in Compiler::argOrReturnTypeForStruct, by IsSingleFloat32Struct.
The implementation is almost same IsHfa and GetHfaType because the logic is very similar.

Fix #6051

@wateret
Copy link
Member Author

wateret commented Jul 8, 2016

/cc @dotnet/jit-contrib @briansull @janvorli @jyoungyun

@CarolEidt
Copy link

I don't think that this should require a new JIT/EE interface method. I think that for the ARM_SOFTFP case, when size == 4, Compiler::argOrReturnTypeForStruct() should use the existing methods (getClassNumInstanceFields, getFieldInClass and getFieldType) to determine whether there is a single field of type float. See Compiler::lvaCanPromoteStructType for how these methods are used to get struct promotion info.

@briansull
Copy link

I am also doing some work in this area.
I will put up my changes on GitHub later on today.

@briansull
Copy link

briansull commented Jul 9, 2016

When HFA are disabled then all structs types are passed using the integer register set.
Why isn't that correct for Soft FP

Please take a look at PR #6198
Where I have completely removed Compiler::argOrReturnTypeForStruct and replaced it with new methods to implement this in a better fashion.

@wateret
Copy link
Member Author

wateret commented Jul 12, 2016

@briansull IMHO it causes type-mismatched gentree. In my example in #6051, it fails with an assertion error. Here is jitdump just before crash.

Generating BB01, stmt 1     Holding variables: []

     (  5,  6) [000017] -------------             *  stmtExpr  void  (top level) (IL   ???...  ???)
N001 (  1,  1) [000035] -------------             |  /--*  dconst    float  1.0000000000000000 $41
N003 (  5,  6) [000016] -A--G---R----             \--*  =         int    $41
N002 (  3,  4) [000013] D---G--N-----                \--*  lclFld    int    V01 tmp0         [+0]
                                                     \--*    float  V01.f1 (offs=0x00) -> V04 tmp3         

Execution Order:
N001 (  1,  1) [000035] -------------             *  dconst    float  1.0000000000000000 $41
N002 (  3,  4) [000013] D---G--N-----             *  lclFld    int    V01 tmp0         [+0]
                                                  *    float  V01.f1 (offs=0x00) -> V04 tmp3         
N003 (  5,  6) [000016] -A--G---R----             *  =         int    $41


Assert failure(PID 3958 [0x00000f76], Thread: 3958 [0x0f76]): Assertion failed '!"Bad IL: Illegal assignment of float into integer!"' in 'HFATest.TestCase:Main():int' (IL size 16)

    File: /home/hanjoung/ws/dxl/dotnet/coreclr/src/jit/codegenlegacy.cpp Line: 11450
    Image: /opt/home/owner/0629/corerun

**** MessageBox invoked, title 'corerun - Assert Failure (PID 3958, Thread 3958/0x0f76)' ****
  Assertion failed '!"Bad IL: Illegal assignment of float into integer!"' in 'HFATest.TestCase:Main():int' (IL size 16)

However for arm(hardfp), HFA handles this case so there is no problem.

@wateret
Copy link
Member Author

wateret commented Jul 12, 2016

@briansull I tested your work(#6198) but I get the same error. Actually I need something like https://github.com/dotnet/coreclr/pull/6198/files#diff-de7f32a5c493a88f0958dcea62ad2f67R524, but FEATURE_HFA is 0 for softfp so IsHfa() always return false. So I think we still need something like this PR.

I'm thinking about implementing this PR again after #6198 is merged. According to @CarolEidt's guide this time.

@briansull
Copy link

You may have to implement something new here.
I have seen various problem crop up when floating point constants are arguments and must be passed using the integer registers.

Compiler::getPrimitiveTypeForStruct may return TYP_FLOAT but is off on softfp.
So we need to check that in another way rather than `IsHfa()`.
Otherwise struct types with single float32 will make inconsistency in gentree.
Which ends up with assert failure "Bad IL: Illegal assignment of float into integer!".

Check is done in Compiler::getPrimitiveTypeForStruct, by isSingleFloat32Struct.

Fix #6051
@wateret
Copy link
Member Author

wateret commented Jul 13, 2016

I have updated the code and its commit message. This resolves 12 subtests of JIT/jit64/hfa/main/testE on arm-softfp.

Please take a look again. @briansull @CarolEidt

@briansull
Copy link

briansull commented Jul 13, 2016

I'm not sure why this is necessary but since this is for ARM_SOFTFP only, I'm OK with it.
I also wonder why a similar issue doesn't exist for a struct that wraps a TYP_DOUBLE with ARM_SOFTFP.

LGTM

@CarolEidt
Copy link

Looks good.
@wateret - is it the case that a struct that wraps a TYP_DOUBLE will be passed or returned by reference?

@CarolEidt CarolEidt merged commit 1a685cb into dotnet:master Jul 13, 2016
@wateret
Copy link
Member Author

wateret commented Jul 14, 2016

@CarolEidt @briansull

In case of TYP_DOUBLE, it is bigger than pointer size so it is handled with RetBuf(It will be shown in gentree as struct not float). A double element wrapped in struct will be handled this way.
Compared with arm-hardfp, this case is processed with HFA so there is no issue like this.

In summary, the assert failure happens only for struct types which has only single float element.

After this patch, gentree changes like below.
from(same as above comment):

Generating BB01, stmt 1     Holding variables: []

     (  5,  6) [000017] -------------             *  stmtExpr  void  (top level) (IL   ???...  ???)
N001 (  1,  1) [000035] -------------             |  /--*  dconst    float  1.0000000000000000 $41
N003 (  5,  6) [000016] -A--G---R----             \--*  =         int    $41
N002 (  3,  4) [000013] D---G--N-----                \--*  lclFld    int    V01 tmp0         [+0]
                                                     \--*    float  V01.f1 (offs=0x00) -> V04 tmp3         

Execution Order:
N001 (  1,  1) [000035] -------------             *  dconst    float  1.0000000000000000 $41
N002 (  3,  4) [000013] D---G--N-----             *  lclFld    int    V01 tmp0         [+0]
                                                  *    float  V01.f1 (offs=0x00) -> V04 tmp3         
N003 (  5,  6) [000016] -A--G---R----             *  =         int    $41

[Assert Failure!]

to:

Generating BB01, stmt 1     Holding variables: []

     (  5,  6) [000017] -------------             *  stmtExpr  void  (top level) (IL   ???...  ???) 
N001 (  1,  1) [000035] -------------             |  /--*  dconst    float  1.0000000000000000 $41
N003 (  5,  6) [000016] -A--G---R----             \--*  =         float  $41  
N002 (  3,  2) [000013] D---G--N-----                \--*  lclVar    float (AX) V04 tmp3     

Execution Order:
N001 (  1,  1) [000035] -------------             *  dconst    float  1.0000000000000000 $41
N002 (  3,  2) [000013] D---G--N-----             *  lclVar    float (AX) V04 tmp3     
N003 (  5,  6) [000016] -A--G---R----             *  =         float  $41  

[Good]

@wateret wateret deleted the fix-6051 branch July 14, 2016 02:22
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
[ARM-softfp/Linux] Get precise type for struct

Commit migrated from dotnet/coreclr@1a685cb
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants