Lower G_UADDE and legalize G_ADD using narrowScalar on MIPS32.
Details
Details
Diff Detail
Diff Detail
Event Timeline
| test/CodeGen/Mips/GlobalISel/legalizer/add.mir | ||
|---|---|---|
| 262–266 | Note: Machine function generated from input mir has different fixedStack than input: fixedStack:
- { id: 0, offset: 16, size: 4, alignment: 8, stack-id: 0, isImmutable: true }
- { id: 1, offset: 20, size: 4, alignment: 4, stack-id: 0, isImmutable: true }
- { id: 2, offset: 24, size: 4, alignment: 8, stack-id: 0, isImmutable: true }
- { id: 3, offset: 28, size: 4, alignment: 4, stack-id: 0, isImmutable: true }Let's look at these two instructions in output: %5:_(p0) = G_FRAME_INDEX %fixed-stack.0 %6:_(s32) = G_LOAD %5(p0) :: (load 4 from %fixed-stack.0, align 0) They correspond to these two in original input %10:_(p0) = G_FRAME_INDEX %fixed-stack.3 %6:_(s32) = G_LOAD %10(p0) :: (load 4 from %fixed-stack.3, align 0) We see that numbers of Vregs are different but that is fine as they are connected with instructions. | |
Comment Actions
Sorry for the belated reply.
In general, the patch is looking good to me. Unfortunately I cannot comment the note about fixedStack because do not know this part of code good enough. Probably you need to extend the list of reviewers or ask a separate question on the llvm-dev mail list.
Comment Actions
I'm not sure either but fwiw the id's being renumbered between input and output sounds like a bug to me. It's going to be difficult to write tests if they can re-number like that.
Comment Actions
Hi, what happens here (when we use -run-pass) is that mir input given in test is not same as MachineFunction given to legalizer. The reason looks to be that constructors for objects are not called in same order.
This is part of MIR input.
name: add_i128
alignment: 2
tracksRegLiveness: true
fixedStack:
- { id: 0, offset: 28, size: 4, alignment: 4, stack-id: 0, isImmutable: true }
- { id: 1, offset: 24, size: 4, alignment: 8, stack-id: 0, isImmutable: true }
- { id: 2, offset: 20, size: 4, alignment: 4, stack-id: 0, isImmutable: true }
- { id: 3, offset: 16, size: 4, alignment: 8, stack-id: 0, isImmutable: true }
body: |
bb.1.entry:
liveins: $a0, $a1, $a2, $a3
%2:_(s32) = COPY $a0
...
%0:_(s128) = G_MERGE_VALUES %2(s32), %3(s32), %4(s32), %5(s32)
%10:_(p0) = G_FRAME_INDEX %fixed-stack.3
%6:_(s32) = G_LOAD %10(p0) :: (load 4 from %fixed-stack.3, align 0)
...
%1:_(s128) = G_MERGE_VALUES %6(s32), %7(s32), %8(s32), %9(s32)
%14:_(s128) = G_ADD %1, %0
...It looks that we parse line by line and call createGenericVirtualRegister (defs in input go %2, %3, %4, %5, %0, while in reconstructed MF they go %0, %1, %2,...).
This is part of reconstructed MF from start of legalizer pass.
# Machine code for function add_i128: IsSSA, NoPHIs, TracksLiveness Frame Objects: fi#-4: size=4, align=8, fixed, at location [SP+16] fi#-3: size=4, align=4, fixed, at location [SP+20] fi#-2: size=4, align=8, fixed, at location [SP+24] fi#-1: size=4, align=4, fixed, at location [SP+28] bb.0.entry: liveins: $a0, $a1, $a2, $a3 %0:_(s32) = COPY $a0 ... %4:_(s128) = G_MERGE_VALUES %0:_(s32), %1:_(s32), %2:_(s32), %3:_(s32) %5:_(p0) = G_FRAME_INDEX %fixed-stack.0 %6:_(s32) = G_LOAD %5:_(p0) :: (load 4 from %fixed-stack.0, align 0) ... %13:_(s128) = G_MERGE_VALUES %6:_(s32), %8:_(s32), %10:_(s32), %12:_(s32) %14:_(s128) = G_ADD %13:_, %4:_ ...
Our scripts for test generation deal with re-numbering of VRegs with names and regular expressions.
When we look at check lines we can figure out that regular expression [0-9]+ in [[COPY:%[0-9]+]] corresponds to %2 as they have same right part of instruction i.e. " = COPY $a0".
Next, if we look at %0:_(s128) = G_MERGE_VALUES %2(s32), %3(s32), %4(s32), %5(s32) we realise that COPY are lowest bits while COPY3 are highest bits of an i128.
Lowest bits of next i128 are in stack with offset 16. If we could see second block of code it would be clear that VReg with name LOAD holds bits from SP+16.
But since we don't, we and could easily associate LOAD with bits form SP+28 as we cannot see how fixedStack looks in reconstructed MF.
I am not sure if this is classified as a bug since check lines are fine as they just save output that current legalizer version produces for given mir input.
If we changed something later and this test failed we could have hard time interpreting why since we lack some information (that fixedStack looks different in reconstructed MF).
One solution could be to have content in %fixed-stack.0 next to an instruction
; MIPS32: [[FRAME_INDEX:%[0-9]+]]:_(p0) = G_FRAME_INDEX %fixed-stack.0 <fixedStack: { id: 0, offset: 16, size: 4, alignment: 8, stack-id: 0, isImmutable: true }>
; MIPS32: [[LOAD:%[0-9]+]]:_(s32) = G_LOAD [[FRAME_INDEX]](p0) :: (load 4 from %fixed-stack.0, align 0) <fixedStack: { id: 0, offset: 16, size: 4, alignment: 8, stack-id: 0, isImmutable: true }>another idea is to have content of fixed stack from output under comments.
---
name: add_i128
alignment: 2
tracksRegLiveness: true
fixedStack:
- { id: 0, offset: 28, size: 4, alignment: 4, stack-id: 0, isImmutable: true }
- { id: 1, offset: 24, size: 4, alignment: 8, stack-id: 0, isImmutable: true }
- { id: 2, offset: 20, size: 4, alignment: 4, stack-id: 0, isImmutable: true }
- { id: 3, offset: 16, size: 4, alignment: 8, stack-id: 0, isImmutable: true }
;fixedStack:
; - { id: 0, offset: 16, size: 4, alignment: 8, stack-id: 0, isImmutable: true }
; - { id: 1, offset: 20, size: 4, alignment: 4, stack-id: 0, isImmutable: true }
; - { id: 2, offset: 24, size: 4, alignment: 8, stack-id: 0, isImmutable: true }
; - { id: 3, offset: 28, size: 4, alignment: 4, stack-id: 0, isImmutable: true }
body: |
...Comment Actions
That's a different issue to the %fixedstack.<id> renumbering, vreg renumbering is expected. MIR is more lenient than LLVM-IR about what it will parse but it still numbers from 0 while printing. I'm surprised by the renumbering of the fixedStack ids because they are identified within the MachineFunction by their id and some of those values have special meanings (e.g. negative ids).
Comment Actions
I see. Then we can agree that renumbering of the fixedStack ids is a bug not connected with this patch.
Are we ready to commit?
Similar to renumbering of vregs, fixedStack objects are created in different order then in original MF and that causes id renumbering.
In order to avoid id renumbering we could change lib/CodeGen/MIRParser/MIRParser.cpp accordingly.
We can create fixedStack objects bottom-up like this:
for (auto RIt = YamlMF.FixedStackObjects.rbegin();
RIt != YamlMF.FixedStackObjects.rend(); ++RIt) {
const auto &Object = *RIt;
...This way fixedStack objects are created in the same order as in the original MF and there is no id renumbering.
fixedStack objects are currently created in top-down order.
for (const auto &Object : YamlMF.FixedStackObjects) {I can prepare another patch to fix renumbering of the fixedStack ids.
Comment Actions
...
I can prepare another patch to fix renumbering of the fixedStack ids.
As to me, I like this way.
Note: Machine function generated from input mir has different fixedStack than input:
fixedStack: - { id: 0, offset: 16, size: 4, alignment: 8, stack-id: 0, isImmutable: true } - { id: 1, offset: 20, size: 4, alignment: 4, stack-id: 0, isImmutable: true } - { id: 2, offset: 24, size: 4, alignment: 8, stack-id: 0, isImmutable: true } - { id: 3, offset: 28, size: 4, alignment: 4, stack-id: 0, isImmutable: true }Let's look at these two instructions in output:
They correspond to these two in original input
We see that numbers of Vregs are different but that is fine as they are connected with instructions.
On the other hand, ids in fixedStack are not connected with content (content with id:3 in input corresponds to content with id:0 in output).
We cannot see the connection between e.g. Vreg of G_LOAD instruction and its offset in test. Looking at fixedStack at input may lead to misinterpretation of this test. Any comments on this?