[PATCH] D65497: [RISCV] Avoid generating AssertZext for RV64 when lowering floating Libcall

Eli Friedman via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Aug 2 11:35:19 PDT 2019


efriedma added a comment.

I think this is semantically the correct fix, but I have some concern about the way the APIs are written.

Also, I'm concerned about the way you're finding the calls to makeLibCall that need to be modified to use "float" mode.  Fixing them one by one as you find examples in assembly, rather than auditing all the calls, is going to lead to a stream of difficult-to-find bugs.



================
Comment at: include/llvm/CodeGen/TargetLowering.h:1834
+                                         bool IsCastFromFloat) const {
+    return IsExtended;
+  }
----------------
Can the target actually do anything useful with the "IsExtended" boolean?  I think it makes more sense to just write something like `IsExtended && shouldExtendTypeInLibCall(Type, IsCastFromFloat)` in the caller.


================
Comment at: include/llvm/CodeGen/TargetLowering.h:3033
+      bool doesNotReturn = false, bool isReturnValueUsed = true,
+      bool isPostTypeLegalization = false) const;
 
----------------
makeLibCall now takes five booleans, four of which are optional.  This is completely unreadable.

I can see a few ways to fix this. One, make the arguments non-optional, and use enum classes to name each one appropriately.  Two, introduce a makeLibCallOptions struct, with some appropriate defaults and methods to override each one, sort of like CallLoweringInfo.  Or three, introduce more entry points with appropriate names.

Also, I think you actually need to separately handle the return value and the argument; for example, float-to-int conversion has a non-extended argument, and a sign-extended result.  I don't think we generate calls to `__fixsfsi` at the moment on rv64; we call `__fixsfdi` instead.  But I'd rather fix the API to do the right thing, given we're changing it anyway.


================
Comment at: test/CodeGen/RISCV/rv64-complex-float.ll:133
+; CHECK-NEXT:    slli a0, s5, 32
+; CHECK-NEXT:    srli a0, a0, 32
+; CHECK-NEXT:    mv a1, a0
----------------
More unnecessary shifts.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65497





More information about the llvm-commits mailing list