[PATCH] D86925: [MachineSink] add one more profitable pattern for sinking

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Hi,

The update with the RegClassWeight is only partially done IMHO.
Each weight should be added/removed from the related register pressure sets. You can take a look at `MachineLICM.cpp` to see how to use these.

I.e., even if the total cost decreases, this doesn't guarantee that we won't go over the limit of a pressure set and thus, we would spill.
E.g.,

  loop:
     smallDef =
     ... = smallDef
     bigDef = smallDef
     // lot of small defs.
    condbr loop

Sinking `bigDef` would be seen profitable since the weight of `bigDef` is greater than `smallDef`. However, extending `smallDef` will make it overlaps with a lot of small defs and may create spilling (remember that `smallDef` and `bigDef` could be on different register banks.)

For now I would suggest to either:

1. Implement the support of pressure sets, or
2. Only do the transformation when the def of the uses are outside the loop. Like you said, these won't have any impact on the register pressure.

Random though: currently MachineLICM only does hoisting. I think it would make sense to have it also do sinking.
Put differently, conceptually, given we are looking at loop invariant here, it would make sense to have this code in `MachineLICM` instead of `MachineSink`.

What do you think?

Cheers,
-Quentin


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86925