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

Tue Apr 14 09:38:48 PDT 2020

PaoloS marked 16 inline comments as done.
PaoloS added inline comments.

================
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))),
----------------
simoncook wrote:
> 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)
Fair point, but LLVM seems to prefer to give priority to the shift. Therefore it changes e.g. 0x55555555 to 0xaaaaaaaa and vice versa in order to compensate the fact that the shift happens before.
It does the same operation.
In the case of GORCI there's also an exchange of the order of the two operands of the OR operation. The result of the operation is eventually ORed with rs1 itself and it seems that LLVM prefers to do this OR only with one of the two operands, the first one, that eventually will be ORed anyway with the other.
Here again, the outcome is the same.

Just as an example I implemented a test following the exact C syntax of the spec:

```
uint32_t _grev1(uint32_t a) {
  return ((a & 0x55555555) << 1) | ((a & 0xaaaaaaaa) >> 1);
}
```

And the pattern that is created is the one I described that privileges the shifts.

I also implemented in C the version that does the shifts first:

```
uint32_t _grev1b(uint32_t a) {
  return ((a << 1) & 0xaaaaaaaa) | ((a >> 1) & 0x55555555);
}
```
And the pattern was the same, with the shifts coming first.

================
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)>;
----------------
simoncook wrote:
> Can we do anything neater than raw ISD::CondCode values here?
If I add for instance the ConCode from the enum, like SETEQ, it breaks.
I could maybe add some macros with similar names, like RVB_SETEQ in this case. But I'm not sure this would be a preferred way to do it. Would it?

Besides if I use the labels SETEQ and similar other riscv codegen tests not related to bitmanip fail.

By having a brief look at other targets like for instance PowerPC I've seen that they use values like SETEQ and others in the pattern matching as I do, but they actually rely on selectcc while RISCV uses its own riscv_selectcc.

================
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),
----------------
simoncook wrote:
> These patterns are the same as the ones in the block above?
Yes same pattern, but LLVM uses a 64 bit value for the CondCode and complains if I don't specify the type in the pattern of riscv_selectcc. for this reason I have to use 2 versions of the same pattern, one with the 32 bit CondCode and the other with the 64 bit one.
If I'm missing something here of course any suggestion is welcome.

================
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))),
----------------
simoncook wrote:
> I think this might have the same shift/and issue and GREVI/GORCI?
It is similar to the case of GREVI/GORCI. In this case though the difference with the description of the operation in the spec:
```
uint32_t shuffle32_stage(uint32_t src, uint32_t maskL, uint32_t maskR, int N)
{
  uint32_t x = src & ~(maskL | maskR);
  x |= ((src << N) & maskL) | ((src >> N) & maskR);
  return x;
}
```
is that the OR-increment of x is carried out not at the end, but with one of the operands of the OR on the right (the first or the second depending on the parenthesis in the implementation), and always with an equivalent outcome. So instead of having something like this:
```
 src & ~(maskL | maskR);
  x |= ((src << N) & maskL) | ((src >> N) & maskR);
```
we have this:
```
 uint32_t x = src & ~(maskL | maskR);
 x = (x | ((src << N) & maskL)) | ((src >> N) & maskR);
```
Actually I noticed that what happens is that x is ORed with the second operand of the OR:
```
 uint32_t x = src & ~(maskL | maskR);
 x = ((src << N) & maskL) | (x | ((src >> N) & maskR));
```
but then I verified that of course that doesn't matter as it is commutative and LLVM selects correctly
```
shfli a0 8
```

================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:529
+  def : Pat<(riscv_selectcc GPR:$rs1, GPR:$rs2, (i32 20), GPR:$rs1, GPR:$rs2),
+            (MIN  GPR:$rs1, GPR:$rs2)>;
+  def : Pat<(umin GPR:$rs1, GPR:$rs2), (MINU GPR:$rs1, GPR:$rs2)>;
----------------
simoncook wrote:
> nitpick: can you remove these double spaces after instruction names
This feature belongs to the MC patch, but I haven't seen this comment here. I'll fix that for the next commit on the encodings, or hopefully once we'll have the official release 1.00

================
Comment at: llvm/test/CodeGen/RISCV/rv32Zbb.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv32 -verify-machineinstrs < %s \
----------------
lewis-revill wrote:
> This comment is not the case for these files anymore right?
Well, I used the tool for the tests, and I see that many other tests left that message. If it doesn't do any harm...

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

https://reviews.llvm.org/D67348