[PATCH] D140460: [RISCV][MC] Add support for experimental zfa extension

[Jun Sha via Phabricator via llvm-commits]

joshua-arch1 marked 2 inline comments as done.
joshua-arch1 added a comment.

In D140460#4020323 <https://reviews.llvm.org/D140460#4020323>, @craig.topper wrote:

> I found this binutils patch that seems to treat integers without "0x" as indices for FLI. https://github.com/a4lg/binutils-gdb/pull/56/files

I used hexadecimal representation with "0x" to distinguish the integer value 1/2/3/4/8/16 from the floatin-point value 1.0/2.0/3.0/4.0/8.0/16.0. To keep consistent with gcc, now I try to remove "0x" and directly use the decimal encoding. That seems to work too.



================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:412
+
+  return -1;
+}
----------------
craig.topper wrote:
> I can't find any caller that checks for the -1 value. Did I miss it?
Sorry I did forget to use this "-1" again. What I'd like to do here is to distinguish between different floating-point immediates and check whether the value can be applied as the operand in fli.s/fli.d/fli.h instruction. I have added this check in isLoadFPImm() in RISCVAsmParser.cpp.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:385
+
+  static std::map<std::pair<int, int>, int> LoadFPImmMap = {
+      {{0b00000001, 0b000}, 1},  {{0b01101111, 0b000}, 2},
----------------
craig.topper wrote:
> All of the values are in order. Can we use a constexpr array of std::pair<uint8_t, uint8_t> and look it up with std::lower_bound? Once we have the iterator we can use std::distance+1 to convert to 1-31.
I'm afraid not. The values in the pair are not in order. 

```
std::lower_bound returns an iterator pointing to the first element in the range [first, last) that is not less than (i.e. greater or equal to) value, or last if no such element is found.
```
We cannot choose a value that can help us get the index.



================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:385
+
+  static std::map<std::pair<int, int>, int> LoadFPImmMap = {
+      {{0b00000001, 0b000}, 1},  {{0b01101111, 0b000}, 2},
----------------
joshua-arch1 wrote:
> craig.topper wrote:
> > All of the values are in order. Can we use a constexpr array of std::pair<uint8_t, uint8_t> and look it up with std::lower_bound? Once we have the iterator we can use std::distance+1 to convert to 1-31.
> I'm afraid not. The values in the pair are not in order. 
> 
> ```
> std::lower_bound returns an iterator pointing to the first element in the range [first, last) that is not less than (i.e. greater or equal to) value, or last if no such element is found.
> ```
> We cannot choose a value that can help us get the index.
> 
I use an array of std::pair<uint8_t, uint8_t> with std::find and std::distance to simplify this function. Thank you for your suggestion.


================
Comment at: llvm/test/MC/RISCV/rv64zfa-valid.s:1
+# RUN: llvm-mc %s -triple=riscv64 -mattr=+experimental-zfa,+d,+zfh -riscv-no-aliases -show-encoding \
+# RUN:     | FileCheck -check-prefixes=CHECK-ASM,CHECK-ASM-AND-OBJ %s
----------------
craig.topper wrote:
> I don't like the duplicated test content with rv32zfa-valid.s
> 
> I believe our usual MC testing is done like this
> 
> rv32zfa-valid.s <- test with both riscv32 and riscv64 command lines.
> rv32zfa-only-valid.s <- test with riscv32 only instructions that aren't not supported by riscv64
Thank you for your advice. I'll modify mt testing, but in some MC testing like Zfh and Zfhmin, rv32 and rv64 are tested in separate files. 


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

https://reviews.llvm.org/D140460