[PATCH] D67348: [RISCV] Add codegen pattern matching for bit manipulation assembly instructions.

[Simon Cook via Phabricator via llvm-commits]

simoncook added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:661
+
+let Predicates = [HasStdExtZbs] in {
+  def : Pat<(and (xor (shl 1, GPR:$rs2), -1), GPR:$rs1),
----------------
These seem inconsistent as to whether they put GPR:$rs1 or the other argument first.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:673
+            (SBCLR GPR:$rs1, GPR:$rs2)>;
+  def : Pat<(and (rotl -2, GPR:$rs2), GPR:$rs1),
+            (SBCLR GPR:$rs1, GPR:$rs2)>;
----------------
Isn't this the same pattern as on line 664?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:682
+let Predicates = [HasStdExtZbp, IsRV32] in {
+  def : Pat<(or (or (and (srl GPR:$rs1, (i32 1)), (i32 0x55555555)), GPR:$rs1),
+                (and (shl GPR:$rs1, (i32 1)), (i32 0xAAAAAAAA))),
----------------
Are these doing the operations in the right order? By my reading the shifts and the and are the wrong way around.

Isn't this the pattern you're trying to match?

```
(or GPR:$rs1, (or (shl (and GPR:$rs1, (i32 0x55555555), (i32 1))),
                  (srl (and GPR:$rs1, (i32 0xaaaaaaaa), (i32 1)))))
```

(The same applies to all the GREVI/GORCI instructions)


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:775
+let Predicates = [HasStdExtZbt, IsRV32] in {
+  def : Pat<(riscv_selectcc GPR:$rs2, (i32 0), (i32 17), GPR:$rs3, GPR:$rs1),
+            (CMOV GPR:$rs1, GPR:$rs2, GPR:$rs3)>;
----------------
Can we do anything neater than raw ISD::CondCode values here?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:795
+  def : Pat<(ctpop GPR:$rs1), (PCNT GPR:$rs1)>;
+  def : Pat<(sra (shl GPR:$rs1, (i32 24)), (i32 24)), (SEXTB GPR:$rs1)>;
+  def : Pat<(sra (shl GPR:$rs1, (i32 16)), (i32 16)), (SEXTH GPR:$rs1)>;
----------------
This line and the one below should be split out to a `HasStdExtZbb, IsRV32` like the 64-bit variants below


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:819
+
+let Predicates = [HasStdExtZbb, IsRV64] in {
+  def : Pat<(riscv_selectcc GPR:$rs1, GPR:$rs2, (i64 20), GPR:$rs1, GPR:$rs2),
----------------
These patterns are the same as the ones in the block above?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:851
+let Predicates = [HasStdExtZbp, IsRV32] in {
+  def : Pat<(or (or (and (shl GPR:$rs1, (i32 8)), (i32 0x00FF0000)),
+                     (and GPR:$rs1, (i32 0xFF0000FF))),
----------------
I think this might have the same shift/and issue and GREVI/GORCI?


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

https://reviews.llvm.org/D67348