This is an archive of the discontinued LLVM Phabricator instance.

[inlineasm] Propagate operand constraints to the backend
ClosedPublic

Authored by sdardis on Jun 22 2016, 12:21 PM.

Details

Summary

When SelectionDAGISel transforms a node representing an inline asm
block, memory constraint information is not preserved. This can cause
constraints to be broken when a memory offset is of the form:

offset + frame index

when the frame is resolved.

By propagating the constraints all the way to the backend, targets can
enforce memory operands of inline assembly to conform to their constraints.

For MIPSR6, some instructions had their offsets reduced to 9 bits from
16 bits such as ll/sc. This becomes problematic when using inline assembly
to perform atomic operations, as an offset can generated that is too big to
encode in the instruction.

Diff Detail

Event Timeline

sdardis updated this revision to Diff 61586.Jun 22 2016, 12:21 PM
sdardis retitled this revision from to [inlineasm][mips] Propagate operand constraints to the backend.
sdardis updated this object.
sdardis added a reviewer: dsanders.
sdardis set the repository for this revision to rL LLVM.
sdardis added a subscriber: llvm-commits.
dsanders requested changes to this revision.Jun 27 2016, 2:59 AM
dsanders edited edge metadata.

This patch is going in the right direction but I think there's a couple extra things we ought to do to round this off:

  • Stop MachineInstr::print() from printing register constraints when the flag do not hold a register constraint.
  • Make MachineInstr::print() print the memory constraint when the flags have one.
  • Add assertions to InlineAsm::getFlagWordForRegClass() and Inline::getFlagWordForMem() to make sure we cannot put the wrong kind of constraint in the relevant flag bits.

I also think the summary needs to mention that frameindex nodes introduce the out of range offsets that are causing the problem. Without the frameindex nodes we would have dealt with the out-of-range offsets during isel.

lib/Target/Mips/MipsSERegisterInfo.cpp
66–93

I don't mind if we do this in a separate patch but we need to cover the various versions of the LL/SC instructions in this switch statement too.

68

Scoping nit: Please move this into the 'case Mips::INLINE_ASM:', adding the necessary braces to allow this.

86

This value depends on the ISA. It should be 9-bit for MIPS32R6/MIPS64R6 and 16-bit for earlier ISA's.

179

This is over 80 columns

test/CodeGen/Mips/inlineasm-constraint_ZC_2.ll
1–2

Please add a pre-R6 test to prove that 9-16 bit offsets are still accepted by the inline assembly for this case.

14–17

For the R6 case, could you check for the addiu too?

This revision now requires changes to proceed.Jun 27 2016, 2:59 AM
sdardis updated this revision to Diff 62209.Jun 29 2016, 5:34 AM
sdardis retitled this revision from [inlineasm][mips] Propagate operand constraints to the backend to [inlineasm] Propagate operand constraints to the backend.
sdardis updated this object.
sdardis edited edge metadata.

Updated as per review comments, retitled and new summary.

dsanders accepted this revision.Jul 6 2016, 5:02 AM
dsanders edited edge metadata.

LGTM with the pre-R6 test and the addiu check

test/CodeGen/Mips/inlineasm-constraint_ZC_2.ll
3–4

This hasn't been done

16–19

This hasn't been done.

This revision is now accepted and ready to land.Jul 6 2016, 5:02 AM
sdardis closed this revision.Jul 18 2016, 7:06 AM

Thanks, committed rL275786.