[PATCH][ARM]Fix urem/srem of i64 for aeabi target (PR 17881)

[Weiming Zhao]

Ping?

The patch won't affect other targets

 

Weiming

 

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

 

From: Weiming Zhao [mailto:weimingz at codeaurora.org] 
Sent: Monday, November 11, 2013 5:45 PM
To: 'Renato Golin'; 'Tim Northover'
Cc: 'llvm-commits'
Subject: RE: [PATCH][ARM]Fix urem/srem of i64 for aeabi target (PR 17881)

 

PS: I grep 'RTLIB::SDIVREM_I64' in Target directory and only ARM defines
them. 

So the code shouldn't affect other existing targets.

 

Weiming

 

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

 

From: Weiming Zhao [mailto:weimingz at codeaurora.org] 
Sent: Monday, November 11, 2013 5:15 PM
To: 'Renato Golin'; 'Tim Northover'
Cc: 'llvm-commits'
Subject: RE: [PATCH][ARM]Fix urem/srem of i64 for aeabi target (PR 17881)

 

Hi Renato & Tim,

 
It's interesting to see there are already different approaches.
 
I actually did something similar with your patch #1 (custom lower DIVMOD64)
and I couldn't get it work either. 
The problem is legalize occurs earlier.
 
And another tricky thing is the second half of the return value
of__aeabi_divmod is needed. 
SDIV/UDIV is easier because no matter __adabi_uldivmod or  "__udivdi3" is
selected, the only difference is the function name. See
(LegalizeIntegerTypes.cpp::ExpandIntRes_SDIV(), where a i64 div is converted
to function call)
 
I read the other patch (devmod-legal) and seems OK. Why doesn't it work?
 You intercept it in ExpandIntRes_SRM too. 
You use getLibcallReturnSize/Value to check if a DivMod is called. 
My solution is check if SREMDIV is available, if it is, use it first. This
follows the same logic of how SREM_I32 works: if the target provides
RTLIB::SDIVREM_XX, then it will be used.
 
Although we changed LegalizeIntegerTypes.cpp, the logic actually follows the
way SDIV/UDIV works.  As long as a target doesn't fill , RTLIB::SDIVREM_I64
(default is 0 in TargetLoweringBase.cpp), they won't be affected.
 
Thanks,
Weiming

 

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

 

From: Renato Golin [mailto:renato.golin at linaro.org] 
Sent: Monday, November 11, 2013 3:38 PM
To: Tim Northover
Cc: Weiming Zhao; llvm-commits
Subject: Re: [PATCH][ARM]Fix urem/srem of i64 for aeabi target (PR 17881)

 

Hi Weiming,

 

I worked on this a bit in the past, and have attached both partial solutions
to your bug, as well as making my own bug a duplicate of yours, so at least
you can understand where we're coming from. Tim helped me review the patches
and understand the problem, so both of us will be reviewing this.

 

thanks,

-renato

 

On 11 November 2013 22:03, Tim Northover <t.p.northover at gmail.com> wrote:

Hi Weiming,


> Please help to review the patch that addresses i64 mod operation for ARM
> aeabi.

Can this be done in ARM's ISelLowering? The problem is that ExpandRem
is *only* the correct implementation for the ARM-specific __aeabi_*
functions. The more generic GNU-style calls have a different
prototype:

   int64_t __divmoddi4(int64_t l, int64_t r, int64_t *rem);

As it stands, any other target deciding to Expand divrem is likely to
get a nasty shock.

Cheers.

Tim.
_______________________________________________
llvm-commits mailing list
llvm-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131120/d6d8aa9d/attachment.html>