Fix mismatched type IND(LCL_VAR_ADDR) folding in RewriteSIMDOperand#27748
Fix mismatched type IND(LCL_VAR_ADDR) folding in RewriteSIMDOperand#27748CarolEidt merged 2 commits intodotnet:masterfrom
Conversation
If the indirection and the variable have different SIMD types then a LCL_FLD node of the appropiate type is needed instead of a LCL_VAR node.
|
/cc @dotnet/jit-contrib |
| { | ||
| BlockRange().Remove(tree); | ||
|
|
||
| addr->SetOper(loadForm(addr->OperGet())); |
There was a problem hiding this comment.
genTreeOps loadForm(genTreeOps addrForm) is unused after that change, could you please delete it?
There was a problem hiding this comment.
Cool, didn't notice it's no longer need. Thanks!
| // If we have GT_IND(GT_LCL_VAR_ADDR) and the var is a SIMD type, | ||
| // replace the expression by GT_LCL_VAR or GT_LCL_FLD. | ||
| GenTree* addr = tree->AsIndir()->Addr(); | ||
| if (addr->OperIsLocalAddr() && comp->isAddrOfSIMDType(addr)) |
There was a problem hiding this comment.
That looks like before it could process both GT_IND(GT_LCL_FLD_ADDR(GT_LCL_FLD)) and GT_IND(GT_LCL_VAR_ADDR(LCL_VAR)), but now it is limited to the second option.
Was it intentional? Does it produce diffs?
There was a problem hiding this comment.
GT_LCL_FLD_ADDR was rejected by isAddrOfSIMDType. That's also the reason I got rid of the call to isAddrOfSIMDType.
| addr->AsLclFld()->gtLclOffs = 0; | ||
| addr->AsLclFld()->gtFieldSeq = FieldSeqStore::NotAField(); | ||
|
|
||
| if (((addr->gtFlags & GTF_VAR_DEF) != 0) && (genTypeSize(simdType) < genTypeSize(lclType))) |
There was a problem hiding this comment.
Could we see genTypeSize(simdType) > genTypeSize(lclType) here? Probably not because you can't do Unsafe.As<Vector3, Vector4>(ref c); , but if yes then you can use bool GenTree::IsPartialLclFld(Compiler* comp) here.
There was a problem hiding this comment.
I assume it's possible, but the result will be undefined. If it's a local variable you may end up overwriting another local variable but that's not something that liveness would ever care about.
The USEASG case is special - if this isn't set correctly, liveness will believe that the entire variable was written to and it will eliminate any previous stores as dead.
BTW, I'm not sure if this particular case is ever hit. It seems technically possible to hit a VAR_DEF but I haven't been able to construct a case showing this so I only added this code out of caution. I hope that eventually we will get rid of the entire function, all this logic should be either in morph or in lowering.
CarolEidt
left a comment
There was a problem hiding this comment.
LGTM - happy to see the elimination of loadForm
If the indirection and the variable have different SIMD types then a LCL_FLD node of the appropiate type is needed instead of a LCL_VAR node.
Fixes #27405