[PATCH] D67148: [LoopVectorize][PowerPC] Estimate int and float register pressure separately in loop-vectorize

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:801
+  /// return the target-provided register class for the provided type.
+  unsigned getRegisterClassForType(bool Vector, Type *Ty = nullptr) const;
+
----------------
wuzish wrote:
> arsenm wrote:
> > I don't like spreading the concept of register classes corresponding to types.
> > 
> > I also don't think register classes as a concept should be leaking out to the IR
> I think it's not the concept of register class in backend. It's an abstraction of register class in backend and just to classify and distinguish different kinds of data residing in different register position to help estimate register pressure.
Yeah, I think that it's pretty important that these are *abstract* register classes - used to establish the mapping between the types of IR values and the number of simultaneous live ranges to which we'd like to limit for some set of those types - it's probably worth stating that explicitly in the description.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7768
   // interleaving.
-  if (!TTI->getNumberOfRegisters(true) && TTI->getMaxInterleaveFactor(1) < 2)
+  if (!TTI->getNumberOfRegisters(TargetTransformInfo::GenericVectorRC) && TTI->getMaxInterleaveFactor(1) < 2)
     return false;
----------------
wuzish wrote:
> hfinkel wrote:
> > wuzish wrote:
> > > hfinkel wrote:
> > > > I think that we can just make a separate function for this:
> > > > 
> > > >   TTI->hasVectorRegisters()
> > > > 
> > > > (and then use that here and in the SLP vectorizer).
> > > I think it's could be like `TTI->getRegisterClassForType(F.getType(), true)` above
> > But above, F.getType() gives back the right scalar type because F is the LSR::Formula. Here F in the function, right? I don't think it makes sense to ask for the register class of the function type.
> Yes. And above case would return nullptr, so we need care about this situation. And here we can left type to be nullptr as default value argument.
I suppose that this makes sense if we assume that `TTI->getRegisterClassForType` is called only on legal types? I'm a bit worried here because, at the IR level, all types are supported and should be legalized into *some* register class. We should better document the expected behavior here one way or the other.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67148/new/

https://reviews.llvm.org/D67148