Page MenuHomePhabricator

[LV] Add TTI::shouldMaximizeVectorBandwidth to allow enabling it per target
ClosedPublic

Authored by kparzysz on Mar 21 2018, 7:09 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

kparzysz created this revision.Mar 21 2018, 7:09 AM

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.

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?

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.

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.

Ok, that's basically this patch as-is. I thought you wanted me to change something.

craig.topper added inline comments.
include/llvm/Analysis/TargetTransformInfo.h
1344 ↗(On Diff #139286)

Is const needed here? Looking at the nearby methods, it seems only getRegisterBitWidth has it.

kparzysz added inline comments.Mar 21 2018, 2:49 PM
include/llvm/Analysis/TargetTransformInfo.h
1344 ↗(On Diff #139286)

No, it's not needed. Should I remove it? What is the best practice here?

craig.topper added inline comments.Mar 21 2018, 3:21 PM
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.

This revision is now accepted and ready to land.Mar 27 2018, 9:00 AM
This revision was automatically updated to reflect the committed changes.