[PATCH] D67128: [X86] Move x86_64 fp128 conversion to libcalls from type legalization to DAG legalization

Wed Sep 4 11:59:40 PDT 2019

craig.topper marked 2 inline comments as done.
craig.topper added a comment.

In D67128#1658096 <https://reviews.llvm.org/D67128#1658096>, @chh wrote:

> I haven't read all code details yet; just some quick questions and comments.
>
> Are the 3 test cases the only ones changed?

Yes these are the only test changes. I did have issues with other tests failing while I was developing this, but this is where we ended up at the end.

> I know that we don't have very good coverage.
>  The ultimate test will be compatibility with Android x86_64 target,
>  which has some unique ABI not the same as AArch64 and normal x86_64.
> 
> Among those 3 changed unit tests, 2 seemed getting longer output code.
>  Could they be improved to be as good as before?

I think with a little bit of work they could be improved. The one in fp128-compare got longer because originally we lowered to __lttf2 and ISD::SETCC. We have a DAG combine that was able to optimize the zext and that setcc to the shr. But now we never create the ISD::SETCC and go directly to a target specific X86ISD::CMP and X86ISD::SETCC. We don't have a DAG combine to optimize that. In the usual case that the rseult of the fcmp is being used by a branch or select we probably still generate the same code. The zext is what is special here.

For the fp128-i128.ll case it looks like  X86TargetLowering::reduceSelectOfFPConstantLoads needs to know that the FP compare is for an f128 type and not f32/f64. The interface doesn't allow for that currently, but could easily be fixed in a follow up.

> I'm surprised that no change is needed to LegalizeFloatTypes.cpp.
>  On the other hand, please keep this change as small as possible.
>  In the worst case, if we find any regression, we will revert it in the Android release.

I think a bunch of changes can be removed from LegalizeFloatTypes.cpp if this patch goes in.

================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:626

-  // Long double always uses X87, except f128 in SSE.
+  // Long double always uses X87.
   if (UseX87) {
----------------
chh wrote:
> This comment is incorrect now. f128 still should be stored in SSE when available.
> 
I've removed the reference to "long double" here. That's a C/C++ source language type which has different sizes on different targets. Better to be agnostic here and just talk in IR types.

================
Comment at: llvm/test/CodeGen/X86/fp128-compare.ll:62
   ret i32 %conv
 ; The 'shrl' is a special optimization in llvm to combine
 ; the effect of 'fcmp olt' and 'zext'. The main purpose is
----------------
I need to remove this comment until we go back to shr.

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

https://reviews.llvm.org/D67128