[PATCH] D70085: [AMDGPU] NFC target dependent requiresUniformRegister refactored out

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:712
+                           MachineFunction &MF, const Value *V) const {
+    return DA && DA->isDivergent(V);
   }
----------------
alex-t wrote:
> arsenm wrote:
> > The DA check could have skipped the virtual call
> Could you explain how it could happen? Any target with no divergent instructions have DA set to nullptr and method returns false.
There's no real reason to call isDivergent if DA is false. The hook should probably take a reference. You also don't need to include the header to TargetLowering (which is widely included), and cuold sink that into the implementation file


================
Comment at: llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp:401
 Register FunctionLoweringInfo::CreateRegs(const Value *V) {
-  return CreateRegs(V->getType(), DA && DA->isDivergent(V) &&
-                    !TLI->requiresUniformRegister(*MF, V));
+  return CreateRegs(V->getType(), TLI->isDivergent(DA, *MF, V));
 }
----------------
This should avoid all calls with no DA available


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:10975-10976
+                                   MachineFunction &MF, const Value *V) const {
+  return !requiresUniformRegister(MF, V) &&
+         TargetLoweringBase::isDivergent(DA, MF, V);
+}
----------------
arsenm wrote:
> requiresUniformRegister is an expensive check (which really should be eliminated entirely), so should be made second only when really necessary
This part is quite important, I had to fix some really bad compile time regressions a few months ago because requiresUniformRegister is extremely slow


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70085