This has not been implemented by any backends, which appear to cover the functionality through getCastInstrCost. Sink what there is in the default implementation into BasicTTI.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/include/llvm/CodeGen/BasicTTIImpl.h | ||
---|---|---|
738 | This doesn't look as effective as the getTLI()->isExtLoad call in getExtCost - missing one use checks etc. |
After turning on asserts, two inlining tests were failing for AArch64 and PowerPC. I've now updated their implementations of getCastInstrCost to pass the Instruction to the default implementation. I've also done the same for the ARM backend.
It may be better to separate out the "add I to getCastInstrCost" change, which is something I agree with but seems to deserve it's own patch to get the details correct.
llvm/test/Analysis/CostModel/ARM/cast.ll | ||
---|---|---|
1669 ↗ | (On Diff #261824) | I don't think this should be free for Neon, as far as I understand. It doesn't fold the extend into the load like it would for MVE or scalar. |
llvm/include/llvm/CodeGen/BasicTTIImpl.h | ||
---|---|---|
728 | I don't believe this will be correct from the vectorizer. It can pass a context instruction that has different types to the final IR due to it truncating at the same time it vectorizes. It is generally unsound to rely on I being accurate. |
llvm/include/llvm/CodeGen/BasicTTIImpl.h | ||
---|---|---|
728 | I'm sure there's going to be many cases like that, and the vectorizer can always choose to not pass the instruction. |
llvm/include/llvm/CodeGen/BasicTTIImpl.h | ||
---|---|---|
728 | Hmm. You may be correct there in the end. I was kind of hoping that we could keep the context instruction but only look at opcodes. I don't think even that will remain truly correct for very long though. I believe that adding this would at least cause inaccuracies in the costmodelling over what we have right now, possibly regressions in some cases. Not passing I through from the vectorizer (and any other place where we couldn't trust the context) would seem to need a different change to me though. | |
llvm/test/Analysis/CostModel/AArch64/cast.ll | ||
30 | Also, and I may be forgetting something because I thought these looked fine the last time I looked, but why would a sext i32->i64 be free? Would it not need a sxtw? |
llvm/test/Analysis/CostModel/AArch64/cast.ll | ||
---|---|---|
30 | We would, this confused me too. AArch64 looks at all the users of the cast and in the case of no users in returns 0! So it completely breaks these tests. I've haven't given enough context to this patch, because I've also added another cast test with users and that hasn't changed. |
I believe that adding this would at least cause inaccuracies in the costmodelling over what we have right now, possibly regressions in some cases.
Don't forget that this is just doing what it did before though and hopefully anyone who is really concerned about performance won't be relying on BasicTTI to get it. There's also D78937 which may help too.
I don't believe this will be correct from the vectorizer. It can pass a context instruction that has different types to the final IR due to it truncating at the same time it vectorizes. It is generally unsound to rely on I being accurate.