Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
I think this is much cleaner than the other way. Your target can return extremely high cost for super-inefficient "less than full vector" loads/stores, right?
Added the author of D33341.
Yes, we do mark those loads as expensive. However, it looks like the condition on line 6158 could potentially eliminate the desired VF if the estimated register pressure is too high.
Between line 6162 and 6263, you can add something like
auto MinVF = TTI.getMinimumVF(SmallestType); if (MinVF > MaxVF) MaxVF = MinVF;
Assuming that your load/store HW can be fooled between different data types of the same size (and I think most if not all are),
we really don't need to send in the actual data type. Just need the element data size. That'll minimize the change w/o losing generality.
I think the rest of the job can be (and should be) handled by your Target CG.
I suggest positioning D44735 as a simple refinement of D33341 and then add the above mentioned MinVF adjustment on top, as a separate patch.
Does that work?
This plan makes sense to me, but I'm not sure what you mean by the above. Do you want me to reopen D33341? The only change there was to enable the flag by default and update failing testcases, I'd like to keep it disabled by default.
By refinement, I meant making it TTI based, keep it disabled by default, and enable it only for your target. Hopefully, that's easy enough for the rest of the community to quickly review/accept and has much lower chances to get reverted.
That alone I think addresses your immediate concerns, and then MinVF adjustment will add additional peace of mind for those targets that need such enforcement.
include/llvm/Analysis/TargetTransformInfo.h | ||
---|---|---|
1344 ↗ | (On Diff #139286) | Is const needed here? Looking at the nearby methods, it seems only getRegisterBitWidth has it. |
include/llvm/Analysis/TargetTransformInfo.h | ||
---|---|---|
1344 ↗ | (On Diff #139286) | No, it's not needed. Should I remove it? What is the best practice here? |
include/llvm/Analysis/TargetTransformInfo.h | ||
---|---|---|
1344 ↗ | (On Diff #139286) | Looks like a lot of the methods in here were added by Chandler in one commit without the const. It looks like quite a few at end were added later with const. I didn't scroll down enough before to see them before. const seems more correct. So I guess leave it. |