[llvm-commits] [PATCH] Fix instruction selection for vduplane from v4f32 to v2f32

[David Peixotto]

Hi James,

Thanks for the reply. Your plan sounds good to me. I definitely don't want to lose this feature, but I just wasn't sure when I would have a chance to fix the way Silviu was suggesting.

I doubt I can get to it before next week, so if you have a chance that would be wonderful :) My patch contains a proper test case that you should be able to reuse for your fix.

Thanks again for your help!

David

-- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation


> -----Original Message-----
> From: James Molloy [mailto:james.molloy at arm.com]
> Sent: Friday, September 28, 2012 9:26 AM
> To: David Peixotto
> Cc: Silviu Baranga; llvm-commits at cs.uiuc.edu
> Subject: RE: [llvm-commits] [PATCH] Fix instruction selection for vduplane
> from v4f32 to v2f32
> 
> Hi David,
> 
> [Myself and Silviu worked on this patch.]
> 
> I don't think that reverting this patch is the right course of action to take.
> You've found a bug in LLVM - the right action is to fix the bug, not just go in
> and rip out the thing that caused it*. Else we'd never make any progress; all
> bugs are related to some change in some way.
> 
> I think the right course of action is in general to follow  the usual process of
> raising a PR (which you've done). That way someone will get around to fixing
> it before the next release.
> 
> For this particular issue, myself or Silviu can look at this next week, but if you
> have time/inclination to fix it beforehand, that'd be a bonus :)
> 
> Cheers,
> 
> James
> 
> *: Assuming the patch in question passed all pre/post commit checks, which
> for LLVM involve at least one code review, regression tests passing and no
> test-suite regressions.
> 
> On Fri, 2012-09-28 at 17:02 +0100, David Peixotto wrote:
> > Hi Silviu,
> >
> >
> >
> > Thanks for the explanation. Now I understand your concerns.
> >
> >
> >
> > I agree that it seems like a good idea to keep the DAG in a form that
> > the instruction selector expects. Unfortunately, I don’t have a lot of
> > time to make that change right now. I’ll see if I can get to it, but
> > I’m not sure I will have the bandwidth anytime soon.
> >
> >
> >
> > Would it be a good idea to revert the commit that introduced the
> > problem? Would it make sense to apply this patch for now until I can
> > move the fix to the higher level?
> >
> >
> >
> > Thanks,
> >
> > David
> >
> >
> >
> > -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> > hosted by The Linux Foundation
> >
> >
> >
> >
> >
> >
> > From: Silviu Baranga [mailto:silbar01 at arm.com]
> > Sent: Thursday, September 27, 2012 10:38 AM
> > To: 'David Peixotto'; James Molloy; llvm-commits at cs.uiuc.edu
> > Subject: RE: [llvm-commits] [PATCH] Fix instruction selection for
> > vduplane from v4f32 to v2f32
> >
> >
> >
> >
> > Hi David,
> >
> >
> >
> > The main problem is that the code written so far (before r163304) is
> > assuming
> >
> > that type of the input value for vduplane is the same as the output
> > type. This is natural,
> >
> > since it reflects the actual vdup instruction.
> >
> >
> >
> > The pattern solution is identical to what I suggest, only it is in a
> > different place.
> >
> > There could be problems that we don’t know about between the
> > build_vector lowering
> >
> > code and the instruction selection phase.
> >
> >
> >
> > Cheers,
> >
> > Silviu
> >
> >
> >
> > From: David Peixotto [mailto:dpeixott at codeaurora.org]
> > Sent: 27 September 2012 17:41
> > To: Silviu Baranga; James Molloy; llvm-commits at cs.uiuc.edu
> > Subject: RE: [llvm-commits] [PATCH] Fix instruction selection for
> > vduplane from v4f32 to v2f32
> >
> >
> >
> >
> > Hi Silviu,
> >
> >
> >
> > Thanks for the suggestion. That sounds like another way to fix the
> > issue. We started seeing failures after r163304 that added extra
> > support for vduplane
> > (http://llvm.org/viewvc/llvm-project?view=rev&revision=163304). That
> > change was a nice feature and I thought it had just missed a few
> > patterns for going from Q to D vectors.
> >
> >
> >
> > Is there a disadvantage to adding these patterns? If I understand
> > correctly, I think the patterns are essentially doing what you
> > suggest, but doing it in the instruction selector itself rather than
> > with code.
> >
> >
> >
> > -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> > hosted by The Linux Foundation
> >
> >
> >
> >
> >
> >
> > From: Silviu Baranga [mailto:silbar01 at arm.com]
> > Sent: Thursday, September 27, 2012 2:02 AM
> > To: 'David Peixotto'; James Molloy; llvm-commits at cs.uiuc.edu
> > Subject: RE: [llvm-commits] [PATCH] Fix instruction selection for
> > vduplane from v4f32 to v2f32
> >
> >
> >
> >
> > Hi David,
> >
> >
> >
> > Wouldn’t it be better to fix the original problem?
> >
> >
> >
> > It seems that the problem is that the BUILD_VECTOR lowering code is
> > creating a VDUPLANE
> >
> > node that takes a v4i32/v4f32 and creates a v2i32/v2f32. It shouldn’t
> > do that. It should extract
> >
> > the D subreg and do a VDUPLANE on that in this case.
> >
> >
> >
> > Cheers,
> >
> > Silviu
> >
> > From: llvm-commits-bounces at cs.uiuc.edu
> > [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of David Peixotto
> > Sent: 27 September 2012 00:55
> > To: llvm-commits at cs.uiuc.edu
> > Subject: [llvm-commits] [PATCH] Fix instruction selection for vduplane
> > from v4f32 to v2f32
> >
> >
> >
> >
> > We are seeing some instruction selection failures for vduplane from
> > larger vectors to smaller vectors. See PR13938:
> > http://llvm.org/bugs/show_bug.cgi?id=13938.
> >
> >
> >
> > A patch to fix this issue is attached.
> >
> >
> >
> > This patch fixes an instruction selection failure for
> >
> > ARMISD::VDUPLANE. The selector was missing patterns for using vdup
> >
> > to initialize a vector when the source vector is a Q-reg and the
> >
> > destination vector is a D-reg. The fix is to add new patterns to
> >
> > allow the selector to properly select this pattern.
> >
> >
> >
> > -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> > hosted by The Linux Foundation
> >
> >
> >
> >
> >
> >
> 
>