[PATCH] D29938: [RISCV 16/n] Support and tests for a variety of additional LLVM IR constructs

[Philip Reames via Phabricator via llvm-commits]

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM w/identified issues fixed before submission.



================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:151
+
+  if (!isPositionIndependent() && !Subtarget->is64Bit()) {
+    SDValue GAHi = DAG.getTargetExternalSymbol(Sym, Ty, RISCVII::MO_HI);
----------------
As per previous comment, use early return idiom here. (required, minor)


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:152
+  if (!isPositionIndependent() && !Subtarget->is64Bit()) {
+    SDValue GAHi = DAG.getTargetExternalSymbol(Sym, Ty, RISCVII::MO_HI);
+    SDValue GALo = DAG.getTargetExternalSymbol(Sym, Ty, RISCVII::MO_LO);
----------------
I think this needs a comment.  Why is emitting the symbol twice the right thing to do?


================
Comment at: test/CodeGen/RISCV/bswap-ctlz-cttz-ctpop.ll:14
+define i16 @test_bswap_i16(i16 %a) {
+; CHECK-LABEL: test_bswap_i16:
+  %tmp = call i16 @llvm.bswap.i16(i16 %a)
----------------
Please add meaningful check lines so we can see instructions.  Using the check generator would be good.


================
Comment at: test/CodeGen/RISCV/sext-zext-trunc.ll:224
+define i16 @trunc_i64_to_i16(i64 %a) {
+; CHECK-LABEL: trunc_i64_to_i16
+  %1 = trunc i64 %a to i16
----------------
Same here


https://reviews.llvm.org/D29938