Handle DelayFree for HW_Category_SIMDByIndexedElement intrinsics

[kunalspathak]

In #107459, we refactored the code to build refpostions for HWIntrinsics tree nodes. As part of that, we removed the correct way of handling of intrinsics that fall under HW_Category_SIMDByIndexedElement (e.g. MLS) and whose operands have restricted registers V0~V15. We previously marked RefPositions for such operands as "delay-free", but after refactoring, we stopped marking it such and as a result we assigned conflicting registers to targetReg as well as one of the 2nd/3rd operand. Because of that, code gen need to do the required move from mov targetReg, op1Reg (whenever targetReg != op1reg), but if targetReg also contains the value of 2nd/3rd operand, the move would overwrite its value.

Here is the portion of changes that changed the behavior.

Fixes: #114358, #114322

[kunalspathak]

cc: @a74nh

[dotnet-policy-service]

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

[kunalspathak]

This might need a backport because this can lead to silent bad codegen issue for lot of intrinsics that fall in HW_Category_SIMDByIndexedElement category.

[a74nh]

Thanks for finding this. To keep in the existing style, could you:

Expand getDelayFreeOperand() to:
GenTree* LinearScan::getDelayFreeOperand(GenTreeHWIntrinsic* intrinsicTree, bool embedded, bool *forceDelay)
Where forceDelay is a return argument.

Move your else if into getDelayFreeOperand() (I guess it'll go into the default case) and if it passes, set *forceDelay=true and return nullptr.

Then in line 1500 ("Only build as delay free use if register types match") add a || forceDelay == true

[kunalspathak]

That was my first choice, but getDelayFreeOperand() is outside this for loop and that will return nullptr even for op1. If that happens, we won't be able to tgtPrefUse = delayUse i.e. we won't be able to prefer the register allocated for op1 for target as well. Usually when targetReg == op1Reg, we avoid an extra move before the RMW instruction.

[a74nh]

Yes, I hadn't really taken the for loop into account properly.

For all these SIMDByIndexedElement intrisnics, will op1 always be a delay operand?

[kunalspathak]

If you see the code, it's not delayed as such, but rather is a way for us to tell that it is the preferred register for target. It is not marked as delay-free.

[kunalspathak]

/azp run Antigen, Fuzzlyn, runtime-coreclr jitstressregs, runtime-coreclr libraries-jitstressregs

[azure-pipelines]

Azure Pipelines successfully started running 4 pipeline(s).

[kunalspathak]

Antigen, Fuzzlyn and libraries-jitstressregs failures are unrelated.
@dotnet/jit-contrib @BruceForstall / @amanasifkhalid PTAL

[kunalspathak]

/ba-g timeout issue

[kunalspathak]

In #107459, we refactored the code to build refpostions for HWIntrinsics tree nodes. As part of that, we removed the correct way of handling of intrinsics that fall under HW_Category_SIMDByIndexedElement (e.g. MLS) and whose operands have restricted registers V0~V15. We previously marked RefPositions for such operands as "delay-free", but after refactoring, we stopped marking it such and as a result we assigned conflicting registers to targetReg as well as one of the 2nd/3rd operand. Because of that, code gen need to do the required move from mov targetReg, op1Reg (whenever targetReg != op1reg), but if targetReg also contains the value of 2nd/3rd operand, the move would overwrite its value.

Here is the portion of changes that changed the behavior.

Fixes: #114358, #114322

[kunalspathak]

cc: @a74nh

[dotnet-policy-service]

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

[kunalspathak]

This might need a backport because this can lead to silent bad codegen issue for lot of intrinsics that fall in HW_Category_SIMDByIndexedElement category.

[a74nh]

Thanks for finding this. To keep in the existing style, could you:

Expand getDelayFreeOperand() to:
GenTree* LinearScan::getDelayFreeOperand(GenTreeHWIntrinsic* intrinsicTree, bool embedded, bool *forceDelay)
Where forceDelay is a return argument.

Move your else if into getDelayFreeOperand() (I guess it'll go into the default case) and if it passes, set *forceDelay=true and return nullptr.

Then in line 1500 ("Only build as delay free use if register types match") add a || forceDelay == true

[kunalspathak]

That was my first choice, but getDelayFreeOperand() is outside this for loop and that will return nullptr even for op1. If that happens, we won't be able to tgtPrefUse = delayUse i.e. we won't be able to prefer the register allocated for op1 for target as well. Usually when targetReg == op1Reg, we avoid an extra move before the RMW instruction.

[a74nh]

Yes, I hadn't really taken the for loop into account properly.

For all these SIMDByIndexedElement intrisnics, will op1 always be a delay operand?

[kunalspathak]

If you see the code, it's not delayed as such, but rather is a way for us to tell that it is the preferred register for target. It is not marked as delay-free.

[kunalspathak]

/azp run Antigen, Fuzzlyn, runtime-coreclr jitstressregs, runtime-coreclr libraries-jitstressregs

[azure-pipelines]

Azure Pipelines successfully started running 4 pipeline(s).

[kunalspathak]

Antigen, Fuzzlyn and libraries-jitstressregs failures are unrelated.
@dotnet/jit-contrib @BruceForstall / @amanasifkhalid PTAL

[kunalspathak]

/ba-g timeout issue

[kunalspathak]

In #107459, we refactored the code to build refpostions for HWIntrinsics tree nodes. As part of that, we removed the correct way of handling of intrinsics that fall under HW_Category_SIMDByIndexedElement (e.g. MLS) and whose operands have restricted registers V0~V15. We previously marked RefPositions for such operands as "delay-free", but after refactoring, we stopped marking it such and as a result we assigned conflicting registers to targetReg as well as one of the 2nd/3rd operand. Because of that, code gen need to do the required move from mov targetReg, op1Reg (whenever targetReg != op1reg), but if targetReg also contains the value of 2nd/3rd operand, the move would overwrite its value.

Here is the portion of changes that changed the behavior.

Fixes: #114358, #114322

[kunalspathak]

cc: @a74nh

[dotnet-policy-service]

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

[kunalspathak]

This might need a backport because this can lead to silent bad codegen issue for lot of intrinsics that fall in HW_Category_SIMDByIndexedElement category.

[a74nh]

Thanks for finding this. To keep in the existing style, could you:

Expand getDelayFreeOperand() to:
GenTree* LinearScan::getDelayFreeOperand(GenTreeHWIntrinsic* intrinsicTree, bool embedded, bool *forceDelay)
Where forceDelay is a return argument.

Move your else if into getDelayFreeOperand() (I guess it'll go into the default case) and if it passes, set *forceDelay=true and return nullptr.

Then in line 1500 ("Only build as delay free use if register types match") add a || forceDelay == true

[kunalspathak]

That was my first choice, but getDelayFreeOperand() is outside this for loop and that will return nullptr even for op1. If that happens, we won't be able to tgtPrefUse = delayUse i.e. we won't be able to prefer the register allocated for op1 for target as well. Usually when targetReg == op1Reg, we avoid an extra move before the RMW instruction.

[a74nh]

Yes, I hadn't really taken the for loop into account properly.

For all these SIMDByIndexedElement intrisnics, will op1 always be a delay operand?

[kunalspathak]

If you see the code, it's not delayed as such, but rather is a way for us to tell that it is the preferred register for target. It is not marked as delay-free.

[kunalspathak]

/azp run Antigen, Fuzzlyn, runtime-coreclr jitstressregs, runtime-coreclr libraries-jitstressregs

[azure-pipelines]

Azure Pipelines successfully started running 4 pipeline(s).

[kunalspathak]

Antigen, Fuzzlyn and libraries-jitstressregs failures are unrelated.
@dotnet/jit-contrib @BruceForstall / @amanasifkhalid PTAL

[kunalspathak]

/ba-g timeout issue

[kunalspathak]

In #107459, we refactored the code to build refpostions for HWIntrinsics tree nodes. As part of that, we removed the correct way of handling of intrinsics that fall under HW_Category_SIMDByIndexedElement (e.g. MLS) and whose operands have restricted registers V0~V15. We previously marked RefPositions for such operands as "delay-free", but after refactoring, we stopped marking it such and as a result we assigned conflicting registers to targetReg as well as one of the 2nd/3rd operand. Because of that, code gen need to do the required move from mov targetReg, op1Reg (whenever targetReg != op1reg), but if targetReg also contains the value of 2nd/3rd operand, the move would overwrite its value.

Here is the portion of changes that changed the behavior.

Fixes: #114358, #114322

[kunalspathak]

cc: @a74nh

[dotnet-policy-service]

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

[kunalspathak]

This might need a backport because this can lead to silent bad codegen issue for lot of intrinsics that fall in HW_Category_SIMDByIndexedElement category.

[a74nh]

Thanks for finding this. To keep in the existing style, could you:

Expand getDelayFreeOperand() to:
GenTree* LinearScan::getDelayFreeOperand(GenTreeHWIntrinsic* intrinsicTree, bool embedded, bool *forceDelay)
Where forceDelay is a return argument.

Move your else if into getDelayFreeOperand() (I guess it'll go into the default case) and if it passes, set *forceDelay=true and return nullptr.

Then in line 1500 ("Only build as delay free use if register types match") add a || forceDelay == true

[kunalspathak]

That was my first choice, but getDelayFreeOperand() is outside this for loop and that will return nullptr even for op1. If that happens, we won't be able to tgtPrefUse = delayUse i.e. we won't be able to prefer the register allocated for op1 for target as well. Usually when targetReg == op1Reg, we avoid an extra move before the RMW instruction.

[a74nh]

Yes, I hadn't really taken the for loop into account properly.

For all these SIMDByIndexedElement intrisnics, will op1 always be a delay operand?

[kunalspathak]

If you see the code, it's not delayed as such, but rather is a way for us to tell that it is the preferred register for target. It is not marked as delay-free.

[kunalspathak]

/azp run Antigen, Fuzzlyn, runtime-coreclr jitstressregs, runtime-coreclr libraries-jitstressregs

[azure-pipelines]

Azure Pipelines successfully started running 4 pipeline(s).

[kunalspathak]

Antigen, Fuzzlyn and libraries-jitstressregs failures are unrelated.
@dotnet/jit-contrib @BruceForstall / @amanasifkhalid PTAL

[kunalspathak]

/ba-g timeout issue

[kunalspathak]

In #107459, we refactored the code to build refpostions for HWIntrinsics tree nodes. As part of that, we removed the correct way of handling of intrinsics that fall under HW_Category_SIMDByIndexedElement (e.g. MLS) and whose operands have restricted registers V0~V15. We previously marked RefPositions for such operands as "delay-free", but after refactoring, we stopped marking it such and as a result we assigned conflicting registers to targetReg as well as one of the 2nd/3rd operand. Because of that, code gen need to do the required move from mov targetReg, op1Reg (whenever targetReg != op1reg), but if targetReg also contains the value of 2nd/3rd operand, the move would overwrite its value.

Here is the portion of changes that changed the behavior.

Fixes: #114358, #114322

[kunalspathak]

cc: @a74nh

[dotnet-policy-service]

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

[kunalspathak]

This might need a backport because this can lead to silent bad codegen issue for lot of intrinsics that fall in HW_Category_SIMDByIndexedElement category.

[a74nh]

Thanks for finding this. To keep in the existing style, could you:

Expand getDelayFreeOperand() to:
GenTree* LinearScan::getDelayFreeOperand(GenTreeHWIntrinsic* intrinsicTree, bool embedded, bool *forceDelay)
Where forceDelay is a return argument.

Move your else if into getDelayFreeOperand() (I guess it'll go into the default case) and if it passes, set *forceDelay=true and return nullptr.

Then in line 1500 ("Only build as delay free use if register types match") add a || forceDelay == true

[kunalspathak]

That was my first choice, but getDelayFreeOperand() is outside this for loop and that will return nullptr even for op1. If that happens, we won't be able to tgtPrefUse = delayUse i.e. we won't be able to prefer the register allocated for op1 for target as well. Usually when targetReg == op1Reg, we avoid an extra move before the RMW instruction.

[a74nh]

Yes, I hadn't really taken the for loop into account properly.

For all these SIMDByIndexedElement intrisnics, will op1 always be a delay operand?

[kunalspathak]

If you see the code, it's not delayed as such, but rather is a way for us to tell that it is the preferred register for target. It is not marked as delay-free.

[kunalspathak]

/azp run Antigen, Fuzzlyn, runtime-coreclr jitstressregs, runtime-coreclr libraries-jitstressregs

[azure-pipelines]

Azure Pipelines successfully started running 4 pipeline(s).

[kunalspathak]

Antigen, Fuzzlyn and libraries-jitstressregs failures are unrelated.
@dotnet/jit-contrib @BruceForstall / @amanasifkhalid PTAL

[kunalspathak]

/ba-g timeout issue

[kunalspathak]

In #107459, we refactored the code to build refpostions for HWIntrinsics tree nodes. As part of that, we removed the correct way of handling of intrinsics that fall under HW_Category_SIMDByIndexedElement (e.g. MLS) and whose operands have restricted registers V0~V15. We previously marked RefPositions for such operands as "delay-free", but after refactoring, we stopped marking it such and as a result we assigned conflicting registers to targetReg as well as one of the 2nd/3rd operand. Because of that, code gen need to do the required move from mov targetReg, op1Reg (whenever targetReg != op1reg), but if targetReg also contains the value of 2nd/3rd operand, the move would overwrite its value.

Here is the portion of changes that changed the behavior.

Fixes: #114358, #114322

[kunalspathak]

cc: @a74nh

[dotnet-policy-service]

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

[kunalspathak]

This might need a backport because this can lead to silent bad codegen issue for lot of intrinsics that fall in HW_Category_SIMDByIndexedElement category.

[a74nh]

Thanks for finding this. To keep in the existing style, could you:

Expand getDelayFreeOperand() to:
GenTree* LinearScan::getDelayFreeOperand(GenTreeHWIntrinsic* intrinsicTree, bool embedded, bool *forceDelay)
Where forceDelay is a return argument.

Move your else if into getDelayFreeOperand() (I guess it'll go into the default case) and if it passes, set *forceDelay=true and return nullptr.

Then in line 1500 ("Only build as delay free use if register types match") add a || forceDelay == true

[kunalspathak]

That was my first choice, but getDelayFreeOperand() is outside this for loop and that will return nullptr even for op1. If that happens, we won't be able to tgtPrefUse = delayUse i.e. we won't be able to prefer the register allocated for op1 for target as well. Usually when targetReg == op1Reg, we avoid an extra move before the RMW instruction.

[a74nh]

Yes, I hadn't really taken the for loop into account properly.

For all these SIMDByIndexedElement intrisnics, will op1 always be a delay operand?

[kunalspathak]

If you see the code, it's not delayed as such, but rather is a way for us to tell that it is the preferred register for target. It is not marked as delay-free.

[kunalspathak]

/azp run Antigen, Fuzzlyn, runtime-coreclr jitstressregs, runtime-coreclr libraries-jitstressregs

[azure-pipelines]

Azure Pipelines successfully started running 4 pipeline(s).

[kunalspathak]

Antigen, Fuzzlyn and libraries-jitstressregs failures are unrelated.
@dotnet/jit-contrib @BruceForstall / @amanasifkhalid PTAL

[kunalspathak]

/ba-g timeout issue

[kunalspathak]

In #107459, we refactored the code to build refpostions for HWIntrinsics tree nodes. As part of that, we removed the correct way of handling of intrinsics that fall under HW_Category_SIMDByIndexedElement (e.g. MLS) and whose operands have restricted registers V0~V15. We previously marked RefPositions for such operands as "delay-free", but after refactoring, we stopped marking it such and as a result we assigned conflicting registers to targetReg as well as one of the 2nd/3rd operand. Because of that, code gen need to do the required move from mov targetReg, op1Reg (whenever targetReg != op1reg), but if targetReg also contains the value of 2nd/3rd operand, the move would overwrite its value.

Here is the portion of changes that changed the behavior.

Fixes: #114358, #114322

[kunalspathak]

cc: @a74nh

[dotnet-policy-service]

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

[kunalspathak]

This might need a backport because this can lead to silent bad codegen issue for lot of intrinsics that fall in HW_Category_SIMDByIndexedElement category.

[a74nh]

Thanks for finding this. To keep in the existing style, could you:

Expand getDelayFreeOperand() to:
GenTree* LinearScan::getDelayFreeOperand(GenTreeHWIntrinsic* intrinsicTree, bool embedded, bool *forceDelay)
Where forceDelay is a return argument.

Move your else if into getDelayFreeOperand() (I guess it'll go into the default case) and if it passes, set *forceDelay=true and return nullptr.

Then in line 1500 ("Only build as delay free use if register types match") add a || forceDelay == true

[kunalspathak]

That was my first choice, but getDelayFreeOperand() is outside this for loop and that will return nullptr even for op1. If that happens, we won't be able to tgtPrefUse = delayUse i.e. we won't be able to prefer the register allocated for op1 for target as well. Usually when targetReg == op1Reg, we avoid an extra move before the RMW instruction.

[a74nh]

Yes, I hadn't really taken the for loop into account properly.

For all these SIMDByIndexedElement intrisnics, will op1 always be a delay operand?

[kunalspathak]

If you see the code, it's not delayed as such, but rather is a way for us to tell that it is the preferred register for target. It is not marked as delay-free.

[kunalspathak]

/azp run Antigen, Fuzzlyn, runtime-coreclr jitstressregs, runtime-coreclr libraries-jitstressregs

[azure-pipelines]

Azure Pipelines successfully started running 4 pipeline(s).

[kunalspathak]

Antigen, Fuzzlyn and libraries-jitstressregs failures are unrelated.
@dotnet/jit-contrib @BruceForstall / @amanasifkhalid PTAL

[kunalspathak]

/ba-g timeout issue

[kunalspathak]

In #107459, we refactored the code to build refpostions for HWIntrinsics tree nodes. As part of that, we removed the correct way of handling of intrinsics that fall under HW_Category_SIMDByIndexedElement (e.g. MLS) and whose operands have restricted registers V0~V15. We previously marked RefPositions for such operands as "delay-free", but after refactoring, we stopped marking it such and as a result we assigned conflicting registers to targetReg as well as one of the 2nd/3rd operand. Because of that, code gen need to do the required move from mov targetReg, op1Reg (whenever targetReg != op1reg), but if targetReg also contains the value of 2nd/3rd operand, the move would overwrite its value.

Here is the portion of changes that changed the behavior.

Fixes: #114358, #114322

[kunalspathak]

cc: @a74nh

[dotnet-policy-service]

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

[kunalspathak]

This might need a backport because this can lead to silent bad codegen issue for lot of intrinsics that fall in HW_Category_SIMDByIndexedElement category.

[a74nh]

Thanks for finding this. To keep in the existing style, could you:

Expand getDelayFreeOperand() to:
GenTree* LinearScan::getDelayFreeOperand(GenTreeHWIntrinsic* intrinsicTree, bool embedded, bool *forceDelay)
Where forceDelay is a return argument.

Move your else if into getDelayFreeOperand() (I guess it'll go into the default case) and if it passes, set *forceDelay=true and return nullptr.

Then in line 1500 ("Only build as delay free use if register types match") add a || forceDelay == true

[kunalspathak]

That was my first choice, but getDelayFreeOperand() is outside this for loop and that will return nullptr even for op1. If that happens, we won't be able to tgtPrefUse = delayUse i.e. we won't be able to prefer the register allocated for op1 for target as well. Usually when targetReg == op1Reg, we avoid an extra move before the RMW instruction.

[a74nh]

Yes, I hadn't really taken the for loop into account properly.

For all these SIMDByIndexedElement intrisnics, will op1 always be a delay operand?

[kunalspathak]

If you see the code, it's not delayed as such, but rather is a way for us to tell that it is the preferred register for target. It is not marked as delay-free.

[kunalspathak]

/azp run Antigen, Fuzzlyn, runtime-coreclr jitstressregs, runtime-coreclr libraries-jitstressregs

[azure-pipelines]

Azure Pipelines successfully started running 4 pipeline(s).

[kunalspathak]

Antigen, Fuzzlyn and libraries-jitstressregs failures are unrelated.
@dotnet/jit-contrib @BruceForstall / @amanasifkhalid PTAL

[kunalspathak]

/ba-g timeout issue