PATCH. Fix ARM Leapcrel definition to make it loop hoistable

[Yin Ma]

Hi Jim,

I have read the discussion about commit r162603. It says 

>>>> Explicitly mark LEApcrel pseudos with hasSideEffects.
>>>> 
>>>> It's not clear that they should be marked as such, but tbb formation
>>> fails if t2LEApcrelJT is hoisted of of a loop.
>>>> 
>>>> This doesn't change the flags on these instructions,
>>>> UnmodeledSideEffects was already inferred from the missing pattern.

-----
LEApcrelJT is completely a different instruction than LEApcrel 
LEApcrelJT is used for Jump Table. LEApcrel is paired with ADR for 
Handling pc relative addressing.  We know hoisting LEApcrelJT 
Will make formation fails but unknown for LEApcrel. In the comment, 
it said ' It's not clear that they should be marked as such' for LEApcrel.
We found LEApcrel could be marked as no side effect and tested
It with many benchmarks without any failure. So I raised this issue
To community. 

Could you like to apply my patch and run your validation suite to 
See what happens?

Thanks,

Yin 

-----Original Message-----
From: Jim Grosbach [mailto:grosbach at apple.com] 
Sent: Monday, October 14, 2013 3:21 PM
To: Yin Ma
Cc: Tim Northover; llvm-commits
Subject: Re: PATCH. Fix ARM Leapcrel definition to make it loop hoistable


On Oct 14, 2013, at 11:20 AM, Yin Ma <yinma at codeaurora.org> wrote:

> Hi Tim,
> 
> It is difficult for me to create a test now.  Sorry about this.
> With this patch, I have run all validations and tests, all passed.
> 
> the ADR and LEApcrel are paired instructions to  handle pc-relative
address.
> 
> ADR has not side effect. If we look at the Definition of  LEApcrel. It 
> has much similar definition like ADR. It should be marked as no side 
> effect.
> 

How does that answer Tim's question? Those were added to solve a very real
bug in the compiler. You need to address why this patch doesn't reintroduce
that bug.

-Jim

> Thanks,
> 
> Yin
> 
> 
> -----Original Message-----
> From: Tim Northover [mailto:t.p.northover at gmail.com]
> Sent: Monday, October 07, 2013 12:15 PM
> To: Yin Ma
> Cc: llvm-commits
> Subject: Re: PATCH. Fix ARM Leapcrel definition to make it loop 
> hoistable
> 
> Hi Yin,
> 
>> This patch enables LEAPCREL to be moved out of a loop because it is 
>> currently marked as hasSideEffects in .td file. Actually, it is not.
>> By reading the comment above this instruction. It should not be 
>> marked hasSideEffects from the very beginning.
> 
> Could you explain why Jakob's comment in r162603's commit message 
> (when he added the "hasSideEffects = 1" line) is no longer valid?
> 
> Also, as usual, tests would be good.
> 
> Cheers.
> 
> Tim.
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits