Arm64/Sve: Remove unwanted insScalableOpts and instructions #103620
Conversation
ghost
added
the
area-CodeGen-coreclr
|
@dotnet/arm64-contrib @TIHan @amanasifkhalid PTAL |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
src/coreclr/jit/emitarm64.cpp
Outdated
| if (isPredicateRegister(dstReg) && isPredicateRegister(srcReg)) | ||
| { | ||
| assert(insOptsNone(opt)); | ||
| if (insOptsNone(opt)) |
There was a problem hiding this comment.
This code is odd. This is because for predicates we always use INS_OPTS_SCALABLE_B for a mov, so the passed in opt value doesn't matter ?
There was a problem hiding this comment.
Yes, so basically, this code operates just on INS_OPTS_SCALABLE_B (and we added single test for that). For other places, if we have to pass INS_OPTS_SCALABLE_B if register types are predicate feels like an overhead, so instead this code expects it to be NONE, in which case it will set the options to INS_OPTS_SCALABLE_B.
There was a problem hiding this comment.
Would something like this work?
if (!insOptsNone(opt))
{
assert(opt == INS_OPTS_SCALABLE_B);
}
opt = INS_OPTS_SCALABLE_B;
I think that shape makes it clearer the if-statement is debug-only code, and ensures opt is always INS_OPTS_SCALABLE_B even if we don't have asserts on to catch improper handling.
There was a problem hiding this comment.
assert(opt == INS_OPTS_SCALABLE_B || insOptsNone(opt));
opt = INS_OPTS_SCALABLE_B;
?
| assert(insOptsScalableStandard(opt)); | ||
| return emitInsSve_R_R_I(INS_sve_pmov, attr, reg1, reg2, 0, opt, sopt); | ||
| } | ||
| if (sopt == INS_SCALABLE_OPTS_TO_PREDICATE) |
There was a problem hiding this comment.
Are there any functions we can remove sopt completely?
There was a problem hiding this comment.
Not sure I understand. Can you please elaborate?
There was a problem hiding this comment.
I think Alan means are there any emitInsSve_* methods that no longer need the sopt parameter, because it's only checking that sopt is INS_SCALABLE_OPTS_NONE? emitInsSve_R_R seems close to that, though we still check sopt with insScalableOptsWithVectorLength at the end of the method.
There was a problem hiding this comment.
Ah ok. Thanks @amanasifkhalid for clarifying. I didn't check, but can do it as a follow-up work.
src/coreclr/jit/gentree.cpp
Outdated
| return true; | ||
| // For some variants, the address is in vector. | ||
| // Return false for such cases. | ||
| return false; |
There was a problem hiding this comment.
I was returning true here because I assumed we needed to track the fact there were addresses in Op2. Is it safe to not track that?
There was a problem hiding this comment.
Re-reading, the OperIsMemoryLoad() contract is to return true if this intrinsic may throw NullReferenceException of address is "null". Now, for certain non-faulting intrinsics, it should return false, but for Gather*, this should return true.
Fixed it.
There was a problem hiding this comment.
the OperIsMemoryLoad() contract is to return true if this intrinsic may throw NullReferenceException of address is "null".
That was the part I wasn't sure of.
Happy with the new version.
|
/ba-g timeout issues |
3 participants
Now that we have separate predicate and vector registers, remove all the unnecessary
insScalableOptsentries and fix some unit tests. Also included a fix forGatherVector*which currently AVs.They are all passing: https://gist.github.com/kunalspathak/797640e95a3f3edfd67ef107c19389ae
Also ran the unit tests and they are all working.
Fixes: #103606