[PATCH] D154083: [AMDGPU] Rematerialize scalar loads

[Piotr Sobczak via Phabricator via llvm-commits]

piotr added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:108
 
 bool SIInstrInfo::isReallyTriviallyReMaterializable(
     const MachineInstr &MI) const {
----------------
This is a pre-existent issue, but I am not very comfortable with the fact that we override `isReallyTriviallyReMaterializable` the way we do. 

It works because we rely on extra checks being present before the method is invoked (for example on calling `LiveRangeEdit::allUsesAvailableAt()`). However, the comment in the base class says `isReallyTriviallyReMaterializable` "must return false if (..) or if it requres any address registers that are not always available."

Having said that, I am not sure if there is a practical solution for that (apart from softening the comment in the base class).


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:111
+  if (isVOP1(MI) || isVOP2(MI) || isVOP3(MI) || isSDWA(MI) || isSALU(MI) ||
+      (isSMRD(MI) && MI.isDereferenceableInvariantLoad())) {
     // Normally VALU use of exec would block the rematerialization, but that
----------------
Ironically, the addition of the check `isDereferenceableInvariantLoad` makes the examples that drive this work not handled, as the S_LOADs are not marked as `dereferenceable` at the moment (but that is another issue).


================
Comment at: llvm/test/CodeGen/AMDGPU/remat-smrd.mir:4
 
 # Case that should really rematerialize
 ---
----------------
The patch handles the positive cases involving loads, and does not affect the negative cases.


================
Comment at: llvm/test/CodeGen/AMDGPU/remat-smrd.mir:4
 
 # Case that should really rematerialize
 ---
----------------
piotr wrote:
> The patch handles the positive cases involving loads, and does not affect the negative cases.
(Need to add a test for shrinking)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154083