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

[Jun Sha via Phabricator via llvm-commits]

joshua-arch1 added a comment.

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

> This patch accepts this `fli.d ft1, 1.1754943508222875079687365372222456778186655567720875215087517062784172594547271728515625e-38` as `fli.d ft1, min` which is incorrect for double.

Any suggestions on how to address this issue? In my implementation, the values in double precision will be converted to single precsion. This issue won't exist for other values, but for "min",  the value will be modified.



================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:409
+  uint8_t Sign = Imm.extractBitsAsZExtValue(1, 63);
+  uint8_t Mantissa = Imm.extractBitsAsZExtValue(3, 49);
+
----------------
craig.topper wrote:
> joshua-arch1 wrote:
> > craig.topper wrote:
> > > You can't use only 3 bits of the mantissa to match an immediate for `isFPImmLegal` in the other patch. It has to be bit exact for the entire 64 bit value.
> > In the encoding table, only the top 3 bit of the mantissa differs. We don't need to use all the bits to distinguish between different values. 3-bit expression will not influence precision.
> This function is used by CodeGen in your other patch. That must check all bits of the value. Looking at only 3 bits aliases many distinct values together. This is why your patch accepted `ret double 0x102F3E9DF9CF94` as "min" when it should only accept `double 0x0010000000000000`.
So it will only affect 'min'? I think there will be no problems for other values if I only use 3 bits.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:409
+  uint8_t Sign = Imm.extractBitsAsZExtValue(1, 63);
+  uint8_t Mantissa = Imm.extractBitsAsZExtValue(3, 49);
+
----------------
joshua-arch1 wrote:
> craig.topper wrote:
> > joshua-arch1 wrote:
> > > craig.topper wrote:
> > > > You can't use only 3 bits of the mantissa to match an immediate for `isFPImmLegal` in the other patch. It has to be bit exact for the entire 64 bit value.
> > > In the encoding table, only the top 3 bit of the mantissa differs. We don't need to use all the bits to distinguish between different values. 3-bit expression will not influence precision.
> > This function is used by CodeGen in your other patch. That must check all bits of the value. Looking at only 3 bits aliases many distinct values together. This is why your patch accepted `ret double 0x102F3E9DF9CF94` as "min" when it should only accept `double 0x0010000000000000`.
> So it will only affect 'min'? I think there will be no problems for other values if I only use 3 bits.
In the binary representation of double 'min' value, the mantissa bits to be 0. That is to say, the first 3 bits are all 0. I'm still wondering why I cannot use only 3 bits to distinguish between other values in double precision. 
If I limit the other bits apart from the first 3 to be 0, I think it can differentiate all the values.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:409
+  uint8_t Sign = Imm.extractBitsAsZExtValue(1, 63);
+  uint8_t Mantissa = Imm.extractBitsAsZExtValue(3, 49);
+
----------------
joshua-arch1 wrote:
> joshua-arch1 wrote:
> > craig.topper wrote:
> > > joshua-arch1 wrote:
> > > > craig.topper wrote:
> > > > > You can't use only 3 bits of the mantissa to match an immediate for `isFPImmLegal` in the other patch. It has to be bit exact for the entire 64 bit value.
> > > > In the encoding table, only the top 3 bit of the mantissa differs. We don't need to use all the bits to distinguish between different values. 3-bit expression will not influence precision.
> > > This function is used by CodeGen in your other patch. That must check all bits of the value. Looking at only 3 bits aliases many distinct values together. This is why your patch accepted `ret double 0x102F3E9DF9CF94` as "min" when it should only accept `double 0x0010000000000000`.
> > So it will only affect 'min'? I think there will be no problems for other values if I only use 3 bits.
> In the binary representation of double 'min' value, the mantissa bits to be 0. That is to say, the first 3 bits are all 0. I'm still wondering why I cannot use only 3 bits to distinguish between other values in double precision. 
> If I limit the other bits apart from the first 3 to be 0, I think it can differentiate all the values.
I have added some logic to check the other bits of the mantissa. Apart from the first 3 bits, the other bits in the encoding table needs to be all 0. 

I tested my CodeGen patch with 0x0010000000000000 and it passed. And as expected, 0x102F3E9DF9CF94 cannot work now.


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

https://reviews.llvm.org/D140460