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

Fri Feb 17 01:52:11 PST 2017

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
>