This is an archive of the discontinued LLVM Phabricator instance.

[CostModel] Remove getExtCost
ClosedPublic

Authored by samparker on Apr 27 2020, 6:06 AM.

Details

Summary

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.

Diff Detail

Event Timeline

samparker created this revision.Apr 27 2020, 6:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2020, 6:06 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
RKSimon added inline comments.Apr 27 2020, 7:39 AM
llvm/include/llvm/CodeGen/BasicTTIImpl.h
716

This doesn't look as effective as the getTLI()->isExtLoad call in getExtCost - missing one use checks etc.

samparker marked an inline comment as done.
samparker added inline comments.
llvm/include/llvm/CodeGen/BasicTTIImpl.h
716

Thanks, I hadn't done that intentionally. I've uploaded that change in D78937 because it stops this patch from looking like an NFC. I'm not sure whether the vectorization changes are good/expected.

samparker updated this revision to Diff 261824.May 4 2020, 8:07 AM
samparker retitled this revision from [NFCI][CostModel] Remove getExtCost to [CostModel] Remove getExtCost.
samparker added reviewers: uweigand, dmgreen.

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

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.

Yeah, fair enough.

samparker added a comment.EditedMay 7 2020, 3:24 AM

Spun out ARM changes into D79561.

AArch64 change: D79562.

samparker updated this revision to Diff 262597.May 7 2020, 3:54 AM

Rebased on top of the arm and aarch64 changes.

samparker retitled this revision from [CostModel] Remove getExtCost to [NFCI][CostModel] Remove getExtCost.May 7 2020, 3:54 AM
samparker updated this revision to Diff 263402.May 12 2020, 5:20 AM
samparker retitled this revision from [NFCI][CostModel] Remove getExtCost to [CostModel] Remove getExtCost.

Rebased with the new aarch64 tests.

spatel accepted this revision.May 20 2020, 6:27 AM

LGTM

This revision is now accepted and ready to land.May 20 2020, 6:27 AM
dmgreen added inline comments.May 20 2020, 7:31 AM
llvm/include/llvm/CodeGen/BasicTTIImpl.h
709

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.

samparker marked an inline comment as done.May 20 2020, 7:45 AM
samparker added inline comments.
llvm/include/llvm/CodeGen/BasicTTIImpl.h
709

I'm sure there's going to be many cases like that, and the vectorizer can always choose to not pass the instruction.

dmgreen added inline comments.May 20 2020, 9:05 AM
llvm/include/llvm/CodeGen/BasicTTIImpl.h
709

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 ↗(On Diff #263402)

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?

samparker marked an inline comment as done.May 20 2020, 10:54 PM
samparker added inline comments.
llvm/test/Analysis/CostModel/AArch64/cast.ll
30 ↗(On Diff #263402)

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.

This revision was automatically updated to reflect the committed changes.