[llvm] r295300 - [ARM] GlobalISel: Legalize 64-bit G_FADD and G_LOAD

Fri Feb 17 06:15:36 PST 2017

r295439.

Diana

On 17 February 2017 at 10:52, Diana Picus <diana.picus at linaro.org> wrote:
> Hi,
>
> That's right, it should be illegal if we don't have the VFP extension,
> that's pretty much what I meant. Now that you mention it, I think I'll
> add that check here already instead of procrastinating.
>
> The idea was that since I'm only supporting a subset of things in the
> instruction selector, I prefer to check the predicates for each
> instruction there and bail out if they're not satisfied. For instance,
> I only added support for VADDS, and I check in the instruction
> selection that both VFP is enabled and that we don't use NEON for
> single precision FP. I thought that if I moved all the checks to the
> legalizer, it would be less clear where they come from, but then again
> nothing's stopping us from moving just some of the checks to the
> legalizer, where they'll have to be in the end anyway. So, bad plan :)
> I'll fix it.
>
> Thanks,
> Diana
>
>
> On 17 February 2017 at 02:29, Quentin Colombet <qcolombet at apple.com> wrote:
>> Hi Diana,
>>
>>
>>> On Feb 16, 2017, at 1:09 AM, Diana Picus via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>>
>>> Author: rovka
>>> Date: Thu Feb 16 03:09:49 2017
>>> New Revision: 295300
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=295300&view=rev
>>> Log:
>>> [ARM] GlobalISel: Legalize 64-bit G_FADD and G_LOAD
>>>
>>> For now we just mark them as legal all the time and let the other passes bail
>>> out if they can't handle it. In the future, we'll want to move more of the
>>> brains into the legalizer.
>>>
>>> Modified:
>>>    llvm/trunk/lib/Target/ARM/ARMLegalizerInfo.cpp
>>>    llvm/trunk/test/CodeGen/ARM/GlobalISel/arm-legalizer.mir
>>>
>>> Modified: llvm/trunk/lib/Target/ARM/ARMLegalizerInfo.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMLegalizerInfo.cpp?rev=295300&r1=295299&r2=295300&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/Target/ARM/ARMLegalizerInfo.cpp (original)
>>> +++ llvm/trunk/lib/Target/ARM/ARMLegalizerInfo.cpp Thu Feb 16 03:09:49 2017
>>> @@ -32,6 +32,7 @@ ARMLegalizerInfo::ARMLegalizerInfo() {
>>>   const LLT s8 = LLT::scalar(8);
>>>   const LLT s16 = LLT::scalar(16);
>>>   const LLT s32 = LLT::scalar(32);
>>> +  const LLT s64 = LLT::scalar(64);
>>>
>>>   setAction({G_FRAME_INDEX, p0}, Legal);
>>>
>>> @@ -39,6 +40,11 @@ ARMLegalizerInfo::ARMLegalizerInfo() {
>>>     setAction({G_LOAD, Ty}, Legal);
>>>   setAction({G_LOAD, 1, p0}, Legal);
>>>
>>> +  // FIXME: This is strictly for loading double-precision floating point values,
>>> +  // if the hardware allows it. We rely on the instruction selector to complain
>>> +  // otherwise.
>>
>> I’m kind of puzzled by this fixme. If you can load 64-bit value, why would you care about marking this illegal?
>>
>> My understanding is that this should be illegal only because of alignment constraint not match on the load or the absence of VFP (or whatever :)) extension.
>>
>> What is your plan when you say “rely on instruction selector to complain”?
>>
>> Cheers,
>> -Quentin
>>
>>> +  setAction({G_LOAD, s64}, Legal);
>>> +
>>>   for (auto Ty : {s1, s8, s16, s32})
>>>     setAction({G_ADD, Ty}, Legal);
>>>
>>> @@ -51,6 +57,7 @@ ARMLegalizerInfo::ARMLegalizerInfo() {
>>>   // FIXME: This is a bit sloppy, but for now we'll just rely on the instruction
>>>   // selector to complain if it doesn't support floating point.
>>>   setAction({G_FADD, s32}, Legal);
>>> +  setAction({G_FADD, s64}, Legal);
>>>
>>>   computeTables();
>>> }
>>>
>>> Modified: llvm/trunk/test/CodeGen/ARM/GlobalISel/arm-legalizer.mir
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/ARM/GlobalISel/arm-legalizer.mir?rev=295300&r1=295299&r2=295300&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/test/CodeGen/ARM/GlobalISel/arm-legalizer.mir (original)
>>> +++ llvm/trunk/test/CodeGen/ARM/GlobalISel/arm-legalizer.mir Thu Feb 16 03:09:49 2017
>>> @@ -11,6 +11,7 @@
>>>   define void @test_legal_loads() { ret void }
>>>
>>>   define void @test_fadd_s32() { ret void }
>>> +  define void @test_fadd_s64() { ret void }
>>> ...
>>> ---
>>> name:            test_sext_s8
>>> @@ -174,11 +175,13 @@ registers:
>>>   - { id: 3, class: _ }
>>>   - { id: 4, class: _ }
>>>   - { id: 5, class: _ }
>>> +  - { id: 6, class: _ }
>>> body:             |
>>>   bb.0:
>>>     liveins: %r0, %r1, %r2, %r3
>>>
>>>     ; These are all legal, so we should find them unchanged in the output
>>> +    ; CHECK-DAG: {{%[0-9]+}}(s64) = G_LOAD %0
>>>     ; CHECK-DAG: {{%[0-9]+}}(s32) = G_LOAD %0
>>>     ; CHECK-DAG: {{%[0-9]+}}(s16) = G_LOAD %0
>>>     ; CHECK-DAG: {{%[0-9]+}}(s8) = G_LOAD %0
>>> @@ -190,6 +193,7 @@ body:             |
>>>     %3(s8)  = G_LOAD %0(p0)
>>>     %4(s1)  = G_LOAD %0(p0)
>>>     %5(p0)  = G_LOAD %0(p0)
>>> +    %6(s64) = G_LOAD %0(p0)
>>>     BX_RET 14, _
>>> ...
>>> ---
>>> @@ -217,3 +221,28 @@ body:             |
>>>     BX_RET 14, _, implicit %r0
>>>
>>> ...
>>> +---
>>> +name:            test_fadd_s64
>>> +# CHECK-LABEL: name: test_fadd_s64
>>> +legalized:       false
>>> +# CHECK: legalized: true
>>> +regBankSelected: false
>>> +selected:        false
>>> +tracksRegLiveness: true
>>> +registers:
>>> +  - { id: 0, class: _ }
>>> +  - { id: 1, class: _ }
>>> +  - { id: 2, class: _ }
>>> +body:             |
>>> +  bb.0:
>>> +    liveins: %d0, %d1
>>> +
>>> +    %0(s64) = COPY %d0
>>> +    %1(s64) = COPY %d1
>>> +    %2(s64) = G_FADD %0, %1
>>> +    ; G_FADD with s64 is legal, so we should find it unchanged in the output
>>> +    ; CHECK: {{%[0-9]+}}(s64) = G_FADD {{%[0-9]+, %[0-9]+}}
>>> +    %d0 = COPY %2(s64)
>>> +    BX_RET 14, _, implicit %d0
>>> +
>>> +...
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>