[PATCH] D102310: [RISCV][CodeGen] Implement IR Intrinsic support for K extension

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCV.td:129
+
+  // zbkb
+  def int_riscv_rev8 : BitManipGPRIntrinsics;
----------------
Capital Z


================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCV.td:130
+  // zbkb
+  def int_riscv_rev8 : BitManipGPRIntrinsics;
+  def int_riscv_brev8 : BitManipGPRIntrinsics;
----------------
rev8 is the same as llvm.bswap.i32 or llvm.bswap.i64. We don't need an intrinsic for it.


================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCV.td:1495
+
+// zknd
+def int_riscv_aes32dsi  : ScalarCryptoByteSelect32;
----------------
Capitalize Z in the comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:282
     setOperationAction(ISD::BSWAP, XLenVT,
                        Subtarget.hasStdExtZbb() ? Legal : Expand);
   }
----------------
You want Zbkb in addition to Zbb here.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZb.td:892
 
+let Predicates = [HasStdExtZbpOrZbkb, IsRV32] in {
 // We treat rev8 as a separate instruction, so match it directly.
----------------
This should stay Zbp only.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZb.td:901
 
-let Predicates = [HasStdExtZbp, IsRV64] in {
+let Predicates = [HasStdExtZbpOrZbkb, IsRV64] in {
 // We treat rev8 as a separate instruction, so match it directly.
----------------
This should stay Zbp only.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZb.td:1196
+let Predicates = [HasStdExtZbkb, IsRV32] in {
+def : PatGpr<int_riscv_rev8, REV8_RV32>;
+def : PatGpr<int_riscv_zip, ZIP_RV32>;
----------------
We don't need int_riscv_rev8. We can use bswap


================
Comment at: llvm/test/CodeGen/RISCV/rv32zbc-zbkc-intrinsic.ll:5
+; RUN: llc -mtriple=riscv32 -mattr=+zbkc -verify-machineinstrs < %s \
+; RUN:   | FileCheck %s -check-prefix=RV32ZBKC
 
----------------
Can we use a single check-prefix?


================
Comment at: llvm/test/CodeGen/RISCV/rv32zbp-zbkb.ll:7
+; RUN: llc -mtriple=riscv32 -mattr=+zbkb -verify-machineinstrs < %s \
+; RUN:   | FileCheck %s -check-prefix=RV32ZBKB
+
----------------
Can we use one check-prefix for Zbp and Zbkb?


================
Comment at: llvm/test/CodeGen/RISCV/rv64zbb-zbp-zbkb.ll:9
+; RUN: llc -mtriple=riscv64 -mattr=+zbkb -verify-machineinstrs < %s \
+; RUN:   | FileCheck %s -check-prefix=RV64ZBKB
 
----------------
Can we use one check-prefix for Zbb, Zbp, and Zbkb?


================
Comment at: llvm/test/CodeGen/RISCV/rv64zbc-zbkc-intrinsic.ll:5
+; RUN: llc -mtriple=riscv64 -mattr=+zbkc -verify-machineinstrs < %s \
+; RUN:   | FileCheck %s -check-prefix=RV64ZBKC
 
----------------
Can we use a single check-prefix?


================
Comment at: llvm/test/CodeGen/RISCV/rv64zbp-zbkb.ll:7
+; RUN: llc -mtriple=riscv64 -mattr=+zbkb -verify-machineinstrs < %s \
+; RUN:   | FileCheck %s -check-prefix=RV64ZBKB
+
----------------
Can we use one check-prefix for Zbp and Zbkb?


================
Comment at: llvm/test/CodeGen/RISCV/rv64zksed-intrinsic.ll:13
+; RV64ZKSED-NEXT:    li a2, 0
+; RV64ZKSED-NEXT:    call llvm.riscv.sm4ks.i64 at plt
+; RV64ZKSED-NEXT:    ld ra, 8(sp) # 8-byte Folded Reload
----------------
Something went wrong here


================
Comment at: llvm/test/CodeGen/RISCV/rv64zksed-intrinsic.ll:29
+; RV64ZKSED-NEXT:    li a2, 1
+; RV64ZKSED-NEXT:    call llvm.riscv.sm4ed.i64 at plt
+; RV64ZKSED-NEXT:    ld ra, 8(sp) # 8-byte Folded Reload
----------------
And here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102310