[PATCH] D136432: [AMDGPU] Combine BFI instructions.

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp:2164
+// b) the constants are disjoint
+// This function gathers all operands, does the neccessary checks and then creates the V_BFI instructions.
+bool AMDGPUDAGToDAGISel::Select_BFI(SDNode *N) {
----------------
jsilvanus wrote:
> It seems you support arbitrarily nested expressions, maybe mention here that the example with three clauses is just an example?
Seconded. The comment as-is is bad because it is too stuck in the original motivation for the code. Comments need to start at a high-level.

We need to take a step back and think about what the generalized and high-level thing is we're doing here. And that is:
```
Match a 32-bit expression of the form

  (X1 & C1) | (X2 & C2) | ... | (Xk & Ck)

where the Ci are constants and select it to a sequence of (k-1) V_BFI_B32 instructions if possible.
```
That needs to be the headline of the comment here. (The original motivation can be mentioned later as a further detail.)

What I glossed over is the bracketing. The code as-is only matches one specific possibility for introducing brackets, which is the one you happened to see in the motivation yous tarted with. But that's the kind of thing where we should do better by default.

What if recursion is on the left instead of on the right? What if it is on both sides?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp:2202-2203
+    } else if (O0.getOpcode() == ISD::AND) {
+      if (O0.getValueType() != O1.getValueType())
+        break;
+
----------------
Can this case happen?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp:2220-2237
+  SDNode *NewNode = nullptr;
+  {
+    auto Base = BFIOps.back();
+    auto Insert = BFIOps[BFIOps.size() - 2];
+    NewNode = CurDAG->getMachineNode(
+        AMDGPU::V_BFI_B32_e64, SDLoc(N), N->getValueType(0),
+        {Insert.X->getOperand(1), Insert.X->getOperand(0),
----------------
This whole code can be simplified by starting with `NewNode = BFIOps.back().X->getOperand(0)` and iterating over `reverse(makeArrayRef(BFIOps).drop_back())`


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp:2239
+
+  if (NewNode != nullptr) {
+    ReplaceNode(N, NewNode);
----------------
Is this check necessary?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136432