[llvm-commits] [Fwd: [PATCH] Add custom UINT_TO_FP lowering from v2i32 to v2f32 in 32-bit mode]

[Eli Friedman]

On Tue, Oct 23, 2012 at 5:52 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
> On Tue, Oct 23, 2012 at 5:38 PM, Michael Liao <michael.liao at intel.com> wrote:
>> On Tue, 2012-10-23 at 17:35 -0700, Eli Friedman wrote:
>>> On Tue, Oct 23, 2012 at 5:24 PM, Michael Liao <michael.liao at intel.com> wrote:
>>> > On Tue, 2012-10-23 at 17:17 -0700, Eli Friedman wrote:
>>> >> On Tue, Oct 23, 2012 at 5:01 PM, Michael Liao <michael.liao at intel.com> wrote:
>>> >> > Sent to wrong list. What am I doing? - Michael
>>> >> >
>>> >> >
>>> >> > ---------- Forwarded message ----------
>>> >> > From: Michael Liao <michael.liao at intel.com>
>>> >> > To: <llvmdev at cs.uiuc.edu>
>>> >> > Cc:
>>> >> > Date: Tue, 23 Oct 2012 16:51:54 -0700
>>> >> > Subject: [PATCH] Add custom UINT_TO_FP lowering from v2i32 to v2f32 in 32-bit mode
>>> >> > Hi
>>> >> >
>>> >> > As 32-bit mode doesn't have 64-bit GPR, the sequence converting v2i32 to
>>> >> > v2f32 is quite inefficient in 32-bit mode. This patch adds the custom
>>> >> > lowering in 32-bit mode. In addition, it teaches DAG combine to
>>> >> > transform (build_vec (Xint2fp x) (Xint2fp y) ..) to (Xint2fp (build_vec
>>> >> > x y)) to reduce the strength on FP conversion unit.
>>> >> >
>>> >> > Thanks for your review
>>> >>
>>> >> +  case ISD::UINT_TO_FP: {
>>> >> +    if (N->getOperand(0).getValueType() != MVT::v2i32)
>>> >> +      return;
>>> >>
>>> >> Please check the result type as well, to make it clear what this code is doing.
>>> >
>>> > Sure, I will add extra checking to not do something wrong.
>>> >
>>> >>
>>> >> Is there any way you can come up with testcases for each individual
>>> >> part of this patch?  It looks like you're making three different
>>> >> changes at the same time.
>>> >
>>> > This patch just adds two major changes. The patch to DAG combine depends
>>> > on the change adding in X86 backend. The test case include both tests: 1
>>> > is trivial converting from v2i32 to v2f32; the other is the case where
>>> > the change to DAG combine is to be applied.
>>>
>>> Okay; please separate into two patches when you commit, then.
>>>
>>> I think the DAGCombine might apply too broadly.  For example, turning
>>> BUILD_VECTOR(UINTTOFP(i32 %x),undef,undef,undef) into
>>> UINTTOFP(BUILD_VECTOR(i32 %x,undef,undef,undef)) is a bad idea on
>>> x86-64.
>>
>> The added DAG combine only works when target claims the UINT_TO_FP on
>> the destination type is supported. In 64-bit mode, UINT_TO_FP is claimed
>> to be 'Expand' and this transformation won't be applied.
>
> Okay... so then this won't transform BUILD_VECTOR(UINTTOFP(i32
> %x),UINTTOFP(i32 %y),UINTTOFP(i32 %z),UINTTOFP(i32 %w))?

Err, nevermind; I guess that's sort of a toss-up in terms of performance anyway.

The one case which seems more like to be an issue is
BUILD_VECTOR(SINTTOFP(i32 %x),undef,undef,undef).  Granted, neither
way is particularly inefficient, but the scalar inttofp is probably a
bit faster if the source value is in an integer register.

-Eli