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

[James Molloy]

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
> 
>  
> 
>  
> 
>