[Patch][AArch32] Fix vsel in compareinst optimization

[Weiming Zhao]

Oops, sorry for the typo. Thanks for catching it.

 

Weiming

 

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

 

From: Quentin Colombet [mailto:qcolombet at apple.com] 
Sent: Thursday, December 05, 2013 5:27 PM
To: Weiming Zhao
Cc: llvm-commits
Subject: Re: [Patch][AArch32] Fix vsel in compareinst optimization

 

Thanks for the updated patch.

 

The logical changes look good but you have a new typo on one CHECK :)

+; CHCEK-NOT: cmp

 

With this typo fixed, LGTM.

 

-Quentin

 

On Dec 5, 2013, at 5:19 PM, Weiming Zhao <weimingz at codeaurora.org> wrote:





Hi Quentin,

 

Thanks for review. You're right. It should give up (return false) when
condition code update is needed.

 

I also added two test cases for such scenario.

 

Thanks,

Weiming

 

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

 

From: Quentin Colombet [ <mailto:qcolombet at apple.com>
mailto:qcolombet at apple.com] 
Sent: Thursday, December 05, 2013 3:56 PM
To: Weiming Zhao
Cc: llvm-commits
Subject: Re: [Patch][AArch32] Fix vsel in compareinst optimization

 

Hi Weiming,

 

The following code does not look correct:

-              Sub->getOperand(2).getReg() == SrcReg)

+              Sub->getOperand(2).getReg() == SrcReg && !IsInstrVSel)

 

Indeed, if the CMP and SUB have different operands order, the VSEL condition
would need to be updated as well if the CMP is removed. Otherwise, we would
not select the right operand, would we?

In other words, I think we are missing an update of the opcode of the VSEL
in that case, since no condition code is available here.

Am I missing something?

 

You have a typo in the tests, this happens twice:

+; CHCK-NOT: cmp

 

The E is missing.

 

Thanks,

-Quentin

 

On Dec 5, 2013, at 3:19 PM, Weiming Zhao < <mailto:weimingz at codeaurora.org>
weimingz at codeaurora.org> wrote:






Hi,

 

Attached patch is to fix bug:  <http://llvm.org/bugs/show_bug.cgi?id=18149>
http://llvm.org/bugs/show_bug.cgi?id=18149

 

Basically, the cmp optimization assumes a MI that uses CPSR has an immediate
MO for ARMCC. But this is not true for VSEL instructions.

 

Thanks,

Weiming

 

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

 

<0001-Bug-18149-AArch32-VSel-instructions-has-no-ARMCC-fie.patch>___________
____________________________________
llvm-commits mailing list
 <mailto:llvm-commits at cs.uiuc.edu> llvm-commits at cs.uiuc.edu
 <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

 

<0001-Bug-18149-AArch32-VSel-instructions-has-no-ARMCC-fie.patch>

 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131205/44ef120f/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Bug-18149-AArch32-VSel-instructions-has-no-ARMCC-fie.patch
Type: application/octet-stream
Size: 4991 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131205/44ef120f/attachment.obj>