On Thu, Feb 2, 2017 at 3:55 AM, James Greenhalgh <james.greenha...@arm.com> wrote: > On Thu, Feb 02, 2017 at 04:03:42AM +0000, Hurugalawadi, Naveen wrote: >> Hi James and Kyrill, >> >> Thanks for the review and comments on the patch. >> >> >> On ILP32 systems this would still use the SImode patterns, >> >> so I suggest you use __builtin_popcountll and >> >> an unsigned long long return type to ensure you always exercise the >> >> 64-bit code. >> >> Sorry for not commenting on this part. >> The issue is that code generates "__popcountdi2" for all the codes in >> testcase >> for LP64 and ILP32 variants. >> __builtin_popcount, __builtin_popcountl and __builtin_popcount. >> >> Hence, modified the patch to check for "popcount". > > I don't understand this comment and how it relates to your updated > patch (which still checks for CNT).
Now I understand Kyrill's comments as Naveen and I did not understand before. What Naveen was trying to test was that there was no longer any call to __popcountdi2 or __popcountsi2 any more for all three: __builtin_popcount, __builtin_popcountl and __builtin_popcountll. Kyrill was asking him to change the test for __builtin_popcountl to __builtin_popcountll even though there was already one test in that file already. Now of course what should change still is the argument types to foo1/foo2 so they take long and long long respectively. He will post a new version of the patch soon. > >> Bootstrapped and regression tested on AArch64-Thunderx-Linux machine. >> >> Please find attached the modified patch and let us know if its okay? > > Could you please clarify what you meant above? The patch looks fine (though > possibly not for Stage 4), but I'm not sure of your intent. What he meant was he tested on aarch64-linux-gnu with no regressions. Is it ok for when stage 1 opens with the change of the testcase? Thanks, Andrew > > Thanks, > James
