[PATCH] D116270: [AMDGPU] Enable divergence-driven XNOR selection

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

Looks reasonable, just some minor comments.



================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:3293
 
+  virtual bool isReassocProfitable(SelectionDAG &DAG, SDValue N0,
+                                   SDValue N1) const {
----------------
rampitec wrote:
> foad wrote:
> > Needs a proper descriptive comment.
> Still needs comment.
That's better than nothing but it still doesn't describe what N0 and N1 are or what reassociation is being controlled. If I understand it is something like: `(op N0:(op N00, N01), N1) -> (op (op N00, N1), N01)` where both `op` are the same associative commutative op, and N01 is a constant.


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:12482
+bool llvm::SITargetLowering::hasMemSDNodeUser(SDNode *N) const {
+
+  SDNode::use_iterator I = N->use_begin(), E = N->use_end();
----------------
This can use any_of and N->uses().


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:12485
+  for (; I != E; ++I) {
+    if (isa<MemSDNode>(*I))
+      return true;
----------------
Would be better to check that the use is the memory address, not (e.g.) the value being stored by a store instruction. But I don't know if there's a simple way to check that.


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.h:454
+
+  virtual bool isReassocProfitable(SelectionDAG &DAG, SDValue N0,
+                                   SDValue N1) const override;
----------------
We don't normally repeat `virtual` on the override -- not sure if it matters.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116270