[PATCH] D125102: [RegAllocGreedy] New hook regClassPriorityTrumpsGlobalness

[Jay Foad via Phabricator via llvm-commits]

foad added a subscriber: stefanp.
foad added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetRegisterInfo.h:57
   /// Classes with a higher priority value are assigned first by register
   /// allocators using a greedy heuristic. The value is in the range [0,63].
   const uint8_t AllocationPriority;
----------------
bjope wrote:
> foad wrote:
> > bjope wrote:
> > > This comment should clarify that if using GreedyRegClassPriorityTrumpsGlobalness then the range is [0,31].
> > > 
> > > When looking into how AllocationPriority is used (by our downstream target vs in-tree targets) I noticed that at least PowerPC is using AllocationPriority>32 to set bit 29 in Prio. So they use that as a way to get a higher prio compared to "global and split ranges" based on the AllocationPriority. So, is setting AllocationPriority > 32 a hackier way to trump globalness already without this patch?
> > Yes, setting AllocationPriority > 32 is definitely a //hackier// way of doing this! I don't like it, because then globalness is ignored even for two live ranges with the same AllocationPriority.
> > 
> > I don't want to document that the range is smaller only if you're using GreedyRegClassPriorityTrumpsGlobalness. Because in both cases, you can use priorities >= 32 if you really want to, and it will clobber some other bit in the Prio value.
> > 
> > Do you know //why// PowerPC uses priorities >= 32? Was it done deliberately to clobber the global bit?
> Right, I also found it a bit ugly that those things overlap.
> 
> I don't know much about PowerPC. It looks like it is deliberate as code comments for example say this:
> ```
>   // Give the VSRp registers a non-zero AllocationPriority. The value is less
>   // than 32 as these registers should not always be allocated before global
>   // ranges and the value should be less than the AllocationPriority - 32 for
>   // the UACC registers. Even global VSRp registers should be allocated after
>   // the UACC registers have been chosen.
>   let AllocationPriority = 2;
> 
> ...
> 
>   // Give the VSRp registers a non-zero AllocationPriority. The value is less
>   // than 32 as these registers should not always be allocated before global
>   // ranges and the value should be less than the AllocationPriority - 32 for
>   // the UACC registers. Even global VSRp registers should be allocated after
>   // the UACC registers have been chosen.
>   let AllocationPriority = 2;
> ```
> 
> And here goes another by-the-way: utils/TableGen/CodeGenRegisters.cpp is verifying that the AllocationPriority values used is in the range [0, 63] so just modifying the comment here would make this comment unsynced with the tablegen implementation.
> 
> Maybe one could say something about values above 31 being special since they would overlap with some other Prio-bits (however, which bits that overlap depend on GreedyRegClassPriorityTrumpsGlobalness).
Agreed, the PowerPC usage looks deliberate. It comes from D105854. @stefanp do you think PowerPC might be interested in using regClassPriorityTrumpsGlobalness //instead of// using AllocationPriority values >= 32?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125102