Arm64 SVE: Do not remove embedded conditional selects #116144
Conversation
Fixes dotnet#116140 When an embedded SVE instrinsic is created, during lowering a conditional select is created to embed the operation. This required to ensure there is a mask for the embedded intrinsic to use. Ensure that the embedding conditional select can only be removed if the embedded intrinsic is also removed.
github-actions
bot
added
the
area-CodeGen-coreclr
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
| @@ -591,6 +591,11 @@ const bool dspGCtbls = true; | |||
| #define DISPTREERANGE(range, t) \ | |||
| if (JitTls::GetCompiler()->verbose) \ | |||
| JitTls::GetCompiler()->gtDispTreeRange(range, t); | |||
| #define LABELEDDISPTREERANGE(label, range, t) \ | |||
There was a problem hiding this comment.
Happy to remove this, but I do think it's useful. Any suggestions for a better name are welcome
|
@kunalspathak |
|
Was having a further think about this. I've reworked Checked spmidiff and there are no changes. I'm happy with this PR now. |
| if (IsEmbeddingMaskOp()) | ||
| { | ||
| assert(this->AsHWIntrinsic()->GetHWIntrinsicId() == NI_Sve_ConditionalSelect); | ||
| return this->AsHWIntrinsic()->Op(2)->OperMayThrow(comp); |
There was a problem hiding this comment.
What if this is user written ConditionalSelect and contains something that throws in op1 or op3? Should we check for those as well?
There was a problem hiding this comment.
Also OperMayThrow should be kept in sync with OperExceptions. It does not make sense for the latter to return an empty set of the former returns true.
There was a problem hiding this comment.
What if this is user written
ConditionalSelectand contains something that throws inop1orop3? Should we check for those as well?
These methods like OperMayThrow, OperRequiresCallFlag etc. do not consider operand nodes. They are meant to recompute the flag for the node itself only.
I would rather see us introducing a new variant of OperMayThrow that takes containment into account and which may be used in the rare case that the backend needs it. It should not need hardcoding to a specific oper type like this.
There was a problem hiding this comment.
I would rather see us introducing a new variant of
OperMayThrow
Done. Not keen on the new name (OperOrEmbeddedChildrenMayThrow())
that takes containment into account and which may be used in the rare case that the backend needs it. It should not need hardcoding to a specific oper type like this.
Specifically, it still checks IsEmbeddingMaskOp() and not containment (as there is a period where the nodes are not yet contained).
What if this is user written
ConditionalSelectand contains something that throws inop1orop3? Should we check for those as well?
Also done.
There was a problem hiding this comment.
Specifically, it still checks IsEmbeddingMaskOp() and not containment (as there is a period where the nodes are not yet contained).
The liveness that is using this runs after lowering. After lowering, all the containment checks should be done. If we are not properly marking these operands as contained during lowering then that seems like a bug. Have we actually introduced another concept of "containment" called "embedding" here?
There was a problem hiding this comment.
The way I interpreted this comment was that I shouldn't be relying on the changes that happen in lowering. Re-reading now I'm not sure if I still agree.
I'm happy to remove the new GTF_HW_EMBEDDING_OP flag and instead OperOrEmbeddedChildrenMayThrow() would check nodes based on containing.
??
There was a problem hiding this comment.
I'm happy to remove the new
GTF_HW_EMBEDDING_OPflag and insteadOperOrEmbeddedChildrenMayThrow()would check nodes based on containing.
Fixed it up to work that way
There was a problem hiding this comment.
I upvoted that because changing OperMayThrow to check child nodes is outside the contract of what OperMayThrow is supposed to do. I think @tannergooding's comment was about the same thing. HIR uses this function to determine if the evaluation of a node itself may throw, and its contract is supposed to exclude child nodes (function header could be clearer on this point). For checking if a subtree rooted at a node may throw we have GTF_EXCEPT. So changing OperMayThrow could indeed be a pessimization for HIR.
However, in LIR things are different because of containment and because of how evaluation works. The flags no longer make sense there because the operands of the nodes are not actually its children, and are their own thing. The exception of course is containment; evaluation of a node with a contained operand does include the effects of its operand. So we need the spiritual equivalent of GTF_EXCEPT for LIR. I think it makes sense to introduce a version of OperMayThrow that does that. I would suggest calling it something like NodeOrContainedOperandsMayThrow().
There was a problem hiding this comment.
I think @tannergooding's comment was about the same thing
Yep. There's quite a bit of nuance between LIR and HIR in some cases and the guarantees we have in place. We generally don't want to make modifications to general IR for something that is LIR only and instead want to have a Lowering::* specific function instead -or- try to work with what LIR may expect, such as by introducing a different LIR specific intrinsic ID (which is what we do for xarch in several cases).
|
/azp run Fuzzlyn |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Before I update the PR, and the results are lost, here are the results of the fuzzlyn tests: 
|
| // Return Value: | ||
| // True if the given operator or contained children may cause an exception | ||
| // | ||
| bool GenTree::NodeOrContainedOperandsMayThrow(Compiler* comp) |
There was a problem hiding this comment.
At least in short term, this should only be for arm64. We are seeing diffs on x64 because this is also affecting x64.
I think it's fine to keep this for x64, despite some small regressions. Alternatively we should figure out how x64 deals with the situation for its contained indirections and then do it the same way for arm64. |
Can this be a follow-up work then, or should it be as part of this PR? I prefer the former. |
I think we should just take this change for all platforms. It's only a handful of regressions on x64, and the logic is equally right everywhere. |
|
@jakobbotsch - do you know what these errors are? I will rerun those legs.
|
|
The superpmi collection after #116484 yesterday failed due to Helix problems, so that's why you're seeing the SPMI failures here. I am rerunning the collection internally. |
|
/azp run runtime-coreclr superpmi-diffs |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/ba-g failures unrelated |
Fixes #116140
When an embedded SVE instrinsic is created, during lowering a conditional select is created to embed the operation. This required to ensure there is a mask for the embedded intrinsic to use.
Ensure that the embedding conditional select can only be removed if the embedded intrinsic is also removed.