[PATCH] D107677: Prevent machine licm if remattable with a vreg use

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added a comment.

In D107677#2941270 <https://reviews.llvm.org/D107677#2941270>, @dmgreen wrote:

> And now has a bad day:

    LE       lr,#innerloop
  ...
    VCTP lr

Is that after this patch or after D106408 <https://reviews.llvm.org/D106408>? It looks more like rematerialization and not hoisting.

> Taking a step back, my understanding of "Trivial Rematerialization" comes from the definition above isTriviallyReMaterializable:
>
>   /// Return true if the instruction is trivially rematerializable, meaning it
>   /// has no side effects and requires no operands that aren't always available.
>   /// This means the only allowed uses are constants and unallocatable physical
>   /// registers so that the instructions result is independent of the place
>   /// in the function.
>   bool isTriviallyReMaterializable(const MachineInstr &MI,
>                                    AAResults *AA = nullptr) const {
>
> So they are expected to only have operands that are available everywhere in the program. That is what makes them trivial to rematerialize. It sounds like D105742 <https://reviews.llvm.org/D105742> (and D87280 <https://reviews.llvm.org/D87280> although I'm not sure it should have) has changed that definition to now include instruction that include virtual uses. Non-trivial rematerialization, if you will. And it is now the responsibility of the caller to ensure that the virtual uses are valid at the point it is being moved. That is what D106396 <https://reviews.llvm.org/D106396> was fixing, and what D106408 <https://reviews.llvm.org/D106408> is extending. Does that sound about right so far?

Yes, this sounds right. Moreover, AMDGPU was allowing it for years, just for few instructions, namely moves.

> If so can we update the docs to match the new behaviour? I'm not sure I would really count it as "trivially rematerializable" anymore, but I don't have a better name for it. From there moving the profitability check into Machine LICM sounds like a fine idea.

Yes, I guess this description is not correct for all targets and will be completely incorrect after D106408 <https://reviews.llvm.org/D106408>. I need to update the comment along with the D106408 <https://reviews.llvm.org/D106408> I suppose. How about this text?

  /// Return true if the instruction is trivially rematerializable, meaning it
  /// has no side effects. Uses of constants and unallocatable physical
  /// registers are always trivial to rematerialize so that the instructions
  /// result is independent of the place in the function. Uses of virtual
  /// registers are allowed but it is caller's responsility to ensure these
  /// operands are valid at the point the instruction is beeing moved.


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

https://reviews.llvm.org/D107677