[PATCH] D41949: [RISCV] implement li pseudo instruction

[Mario Werner via Phabricator via llvm-commits]

niosHD added a comment.

Hi Alex, thank you very much for your comments!

I will address all of them in my next revision. Unfortunately, I am really busy at the moment and will not be able to join the sync up call tomorrow. However, I expect that I can provide the new revision at the beginning of next week.

Best,
Mario



================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVMCPseudoExpansion.cpp:30
+// emitted and is not solely the result of zero extension due to SLLI.
+int findFirstSetBit(int64_t Value, int StartOffset = 0) {
+  Value >>= StartOffset;
----------------
asb wrote:
> Can we not avoid this and use findFirstSet from MathExtras? Unless I'm missing something, you could just mask out the first 12 bits at the call-site and so avoid the need for the 'StartOffset' parameter.
I will have a look, with an additional if at the call site it should probably work. If I remember correctly, having this function is more or less an remainder of an older revision of the patch where Value was checked for 0 and -1.



================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVMCPseudoExpansion.cpp:73-74
+
+  // If more than 32 bits have to be emitted, schedule an ADDI instruction which
+  // handles 12 bits and deal with the remaining bits recursively.
+  int64_t Hi = (Value + 0x800);
----------------
asb wrote:
> The comment describing how emitting 32-bit constants works was fantastic - it would be nice to expand this comment to a similar level of detail. The comment doesn't quite seem to match the behaviour either, as in the implementation emitRISCVLoadImm is called recursively before emitting any other instructions.
I will try to expand/improve the comment to make it clearer. The basic idea was to convey that, at this point, it is already fixed that an ADDI is going to be emitted (hence scheduled). The actual emission, on the other hand, is performed after the recursive all returns.

However, I obviously failed at expressing this and will try again. ;)


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVMCPseudoExpansion.cpp:77
+  int LsbIndex = findFirstSetBit(Hi, 12);
+  Hi >>= LsbIndex;
+
----------------
asb wrote:
> Do you rely on this being an arithmetic right shift? I'm not 100% sure if the  C++ standard guarantees that.
Yes, I rely on that and it seems indeed not guaranteed by the standard. I can add an additional `SignExtend64` call to make it clear. However, the implementation of `SignExtend64` relies right shifts being arithmetic too.


https://reviews.llvm.org/D41949