[GISel] Fix narrowScalar() for big endian targets

[Kai]

Summary

The function narrowScalar() in the LegalizerHelper.cpp does not work correctly
for big endian targets. Breaking down a constant for a big endian target requires
that the big end is stored first. However, the current code does not do this.

This affects all big-endian targets, but the m68k target is the only big endian
target currently implementing GlobalISel.

[myhsu]

llvm/test/CodeGen/M68k/GlobalISel/constant.ll

7

Could you include checks on what registers these values are copied into?

[myhsu]

Thank you for sending this patch, it's always good to see love on big endian support in GlobalISel.

Though the thing you try to fix here seems to be more complicated than it looked. First, this is the MIR output of constant.ll before applying your patch:

body:             |
  bb.1 (%ir-block.0):
    %3:_(s32) = G_CONSTANT i32 51966
    %4:_(s32) = G_CONSTANT i32 0
    $d1 = COPY %3(s32)
    $d0 = COPY %4(s32)
    RTS implicit $d1, implicit $d0

Here is the assembly code:

; %bb.0:
        move.l  #51966, %d1
        move.l  #0, %d0
        rts

This is actually more or less what we want. The reason I say "more or less" is because m68k doesn't really specify the calling convention of returning 64-bit integers (at least I don't find any in SysV ABI). For Linux/GCC ABI, which is the primary ABI we're supporting now, it puts Lo part in $d1 and Hi part in $d0, same as the snippet above. Here is the assembly code if you compile the equivalent C code using m68k-linux-gnu-gcc-9:

link.w %fp,#0
sub.l %a0,%a0
move.l #51966,%a1
move.l %a0,%d0
move.l %a1,%d1
unlk %fp
rts

But here is the catch: the ordering shown above was a product of D116877 by @0x59616e, which honored the endianness when splitting a 64-bit value. In other words, we only start to worry about endianness of integer parts (e.g. Hi and Lo parts) upon materializing them into register pairs (endianness is meaningless when it's not stored in memory, but register pairs that carry integer parts are special cases where people usually put parts in registers that follows endianness).
However, after this patch is applied, it effectively cancels out D116877 because integer parts are swapped again.

So now the question is: who is responsible for taking care of endianness when it comes to integer parts? In SelectionDAGISel, particularly its type legalizer, such task is mostly discussed case-by-case: vector types definitely needs to be aware of endianness, some operations like bitcast also needs to do byte swapping if needed. But I don't see similar things happen on constant (link). Another place that considers endianness is SelecitonDAGBuilder while it's generating Copy nodes (e.g. CopyToReg), which is similar to D116877 where we take care of endianness on integer parts upon materializing them into physical registers.

The bottom line is that maybe this is not the right place to reorder narrowed integer parts according to endianness. But I'm genuinely becoming more confused by this seemly simple endianness issue so it will be great if folks here who are more familiar with this topic can give some comments.

[0x59616e]

FYI, this patch also cancels out the effect of D116931.

[Kai]

Thanks for the insight! The architecture I am working on (ppc64) uses register pairs to pass integers. Apology for not producing a proper test case for m68k.
Currently my code is also very basic, so I might missed something. I'll check out the changes you pointed to, to get a better understanding.

So now the question is: who is responsible for taking care of endianness when it comes to integer parts?

Agree, that's the underlying question.

[arsenm]

llvm/test/CodeGen/M68k/GlobalISel/constant.ll

2

Probably should be a mir->mir test

11

Can you test a few more values and edge cases?

[arsenm]

Needs more tests

llvm/test/CodeGen/M68k/GlobalISel/constant.ll

1

Should use generated checks

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp

     int NumParts = TotalSize / NarrowSize;
 
     SmallVector<Register, 4> PartRegs;
+    bool IsBigEndian = MIRBuilder.getDataLayout().isBigEndian();
     for (int I = 0; I != NumParts; ++I) {
       unsigned Offset = I * NarrowSize;
+      if (IsBigEndian)
+        Offset = TotalSize - NarrowSize - Offset;
       auto K = MIRBuilder.buildConstant(NarrowTy,
                                         Val.lshr(Offset).trunc(NarrowSize));
       PartRegs.push_back(K.getReg(0));

     if (LeftoverBits != 0) {
       LeftoverTy = LLT::scalar(LeftoverBits);
       auto K = MIRBuilder.buildConstant(
           LeftoverTy,
-        Val.lshr(NumParts * NarrowSize).trunc(LeftoverBits));
+          Val.lshr(!IsBigEndian * NumParts * NarrowSize).trunc(LeftoverBits));
       LeftoverRegs.push_back(K.getReg(0));
     }
 

llvm/test/CodeGen/M68k/GlobalISel/constant.ll

+; RUN: llc -march=m68k -global-isel -stop-after=legalizer -simplify-mir < %s | FileCheck %s
+
+; CHECK: name: func
+; CHECK: legalized:       true
+; CHECK: body:
+; CHECK:    {{%[0-9]}}:_(s32) = G_CONSTANT i32 0
+; CHECK:    {{%[0-9]}}:_(s32) = G_CONSTANT i32 51966
+
+define i64 @func() {
+  ret i64 51966 ; 0xcafe
+}