[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: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))),
----------------
PaoloS wrote:
> 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.
> 
Ok, I hadn't noticed that the constants were the other way round to what I'd expect. If this is the canonical pattern SelectionDAG wants to match then indeed, use that pattern.


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

https://reviews.llvm.org/D67348