llvm dev - Mar 2015 - [LLVMdev] Inline Assembly: Memory constraints with offsets

> From: Krzysztof Parzyszek [kparzysz at codeaurora.org]
> On 3/4/2015 10:30 AM, Daniel Sanders wrote:
> >
> > Partial support for ZC is in my working copy at the moment. I've
attached my WIP patches.
> 
> Should have guessed that, ha.
> 
> I've looked into this.  My idea was to expand the single address
operand
> of the inline-asm SDNode into two: base pointer and offset.
> 
> I haven't found a place where all needed information would be readily
> available, and where the common code would do all the work based on some
> target-specific hook.  Failing that, my last resort approach would be to
> have a pre-isel pass that expands the inline-asm, potentially changing
> the constraints to reflect the connection between the register and the
> constant input operands (needed for the asm-printer).
> 
> Not sure if this is of any help.
> 
> -Krzysztof
> 
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
I've had some luck with this. It turns out that the ISD::INLINE_ASM node has
a flag operand corresponding to each operand in the source code. It's used
for storing things like the matched operand number for the [0-9]+ constraints, 
the register class id for register constraints, etc. At first glance, the
encoding looks full but it appears that when the lowest 3 bits are Kind_Mem
(meaning it's a memory constraint), the upper 16-bits are never used.
I've therefore used this gap to store a constraint ID which can then be
inspected during instruction selection. The mapping from constraint string to
constraint id is provided by a target specific hook with unmapped constraints
causing a failure (currently an assertion but it should be llvm_unreachable). I
now have the Mips 'ZC' constraint supporting 12-bit offsets while
'm' continues to lack support for offsets. This isn't the correct
behaviour for 'ZC' or for 'm' but it does at least demonstrate
that different memory constraints can now behave differently. My WIP patch is at
http://reviews.llvm.org/D8160. It's going to be split up into multiple
patches and the Mips portion is going to be corrected before I upstream it.
I'll also check the clang tests to see if I've missed any supported
constraints.

There's a few odd target specific things in the current code so I've
CC'd the code owners. I couldn't find a code owner for PowerPC or MSP430
in CODE_OWNERS.txt though.

The Hexagon and X86 backends both had code that attempted to implement different
behaviour for the 'o' and 'v' memory constraints but this code
has been unused so far because the backend always received the code 'm'.
I attempted to use the existing implementations for these but this causes test
failures so I've left them behaving like 'm'.

The AArch64, ARM, PowerPC, and SystemZ backends have tests for a few constraints
that currently behave like 'm'. I assume that keeping the 'm'
behaviour is ok for now.

There's also a change to the MSP430 backend that should be non-functional
but I ought to mention. The change is that the inline assembly flags are now i32
instead of a pointer-sized int. This is because it will attempt to store a
32-bit value in an i16 otherwise.

Daniel Sanders <Daniel.Sanders at imgtec.com> wrote on 09.03.2015
11:22:02:
> I've had some luck with this. It turns out that the ISD::INLINE_ASM
> node has a flag operand corresponding to each operand in the source
> code. It's used for storing things like the matched operand number
> for the [0-9]+ constraints,  the register class id for register
> constraints, etc. At first glance, the encoding looks full but it
> appears that when the lowest 3 bits are Kind_Mem (meaning it's a
> memory constraint), the upper 16-bits are never used. I've therefore
> used this gap to store a constraint ID which can then be inspected
> during instruction selection. The mapping from constraint string to
> constraint id is provided by a target specific hook with unmapped
> constraints causing a failure (currently an assertion but it should
> be llvm_unreachable). I now have the Mips 'ZC' constraint
supporting
> 12-bit offsets while 'm' continues to lack support for offsets.
This
> isn't the correct behaviour for 'ZC' or for 'm' but it
does at least
> demonstrate that different memory constraints can now behave
> differently. My WIP patch is at http://reviews.llvm.org/D8160. It's
> going to be split up into multiple patches and the Mips portion is
> going to be corrected before I upstream it. I'll also check the
> clang tests to see if I've missed any supported constraints.
>
> There's a few odd target specific things in the current code so
I've
> CC'd the code owners. I couldn't find a code owner for PowerPC or
> MSP430 in CODE_OWNERS.txt though.
The code owner for PowerPC is Hal Finkel (added on CC).
> The Hexagon and X86 backends both had code that attempted to
> implement different behaviour for the 'o' and 'v' memory
constraints
> but this code has been unused so far because the backend always
> received the code 'm'. I attempted to use the existing
> implementations for these but this causes test failures so I've left
> them behaving like 'm'.
>
> The AArch64, ARM, PowerPC, and SystemZ backends have tests for a few
> constraints that currently behave like 'm'. I assume that keeping
> the 'm' behaviour is ok for now.
Sure.  Once the infrastructure is in, I'll update SystemZ to make use
of the additional constraints.   Thanks for taking care of this!


Bye,
Ulrich

----- Original Message -----
> From: "Ulrich Weigand" <Ulrich.Weigand at de.ibm.com>
> To: "Daniel Sanders" <Daniel.Sanders at imgtec.com>
> Cc: adasgupt at codeaurora.org, "evan cheng" <evan.cheng at
apple.com>, "Krzysztof Parzyszek" <kparzysz at
codeaurora.org>,
> llvmdev at cs.uiuc.edu, nrotem at apple.com, "t p northover"
<t.p.northover at gmail.com>, "Hal Finkel" <hfinkel at
anl.gov>
> Sent: Monday, March 9, 2015 9:30:26 AM
> Subject: RE: [LLVMdev] Inline Assembly: Memory constraints with offsets
> 
> Daniel Sanders <Daniel.Sanders at imgtec.com> wrote on 09.03.2015
> 11:22:02:
> 
> > I've had some luck with this. It turns out that the
ISD::INLINE_ASM
> > node has a flag operand corresponding to each operand in the source
> > code. It's used for storing things like the matched operand number
> > for the [0-9]+ constraints,  the register class id for register
> > constraints, etc. At first glance, the encoding looks full but it
> > appears that when the lowest 3 bits are Kind_Mem (meaning it's a
> > memory constraint), the upper 16-bits are never used. I've
> > therefore
> > used this gap to store a constraint ID which can then be inspected
> > during instruction selection. The mapping from constraint string to
> > constraint id is provided by a target specific hook with unmapped
> > constraints causing a failure (currently an assertion but it should
> > be llvm_unreachable). I now have the Mips 'ZC' constraint
> > supporting
> > 12-bit offsets while 'm' continues to lack support for
offsets.
> > This
> > isn't the correct behaviour for 'ZC' or for 'm'
but it does at
> > least
> > demonstrate that different memory constraints can now behave
> > differently. My WIP patch is at http://reviews.llvm.org/D8160.
It's
> > going to be split up into multiple patches and the Mips portion is
> > going to be corrected before I upstream it. I'll also check the
> > clang tests to see if I've missed any supported constraints.
> >
> > There's a few odd target specific things in the current code so
> > I've
> > CC'd the code owners. I couldn't find a code owner for PowerPC
or
> > MSP430 in CODE_OWNERS.txt though.
> 
> The code owner for PowerPC is Hal Finkel (added on CC).
I'd like to second Uli's comment: Thanks for working on this! This is a
code quality issue that affects PowerPC too.

 -Hal
> 
> > The Hexagon and X86 backends both had code that attempted to
> > implement different behaviour for the 'o' and 'v'
memory
> > constraints
> > but this code has been unused so far because the backend always
> > received the code 'm'. I attempted to use the existing
> > implementations for these but this causes test failures so I've
> > left
> > them behaving like 'm'.
> >
> > The AArch64, ARM, PowerPC, and SystemZ backends have tests for a
> > few
> > constraints that currently behave like 'm'. I assume that
keeping
> > the 'm' behaviour is ok for now.
> 
> Sure.  Once the infrastructure is in, I'll update SystemZ to make use
> of the additional constraints.   Thanks for taking care of this!
> 
> 
> Bye,
> Ulrich
> 
> 
-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory

On 3/9/2015 5:22 AM, Daniel Sanders wrote:
>
> I've had some luck with this. It turns out that the ISD::INLINE_ASM
node has a flag operand corresponding to each operand in the source code.
It's used for storing things like the matched operand number for the [0-9]+
constraints,  the register class id for register constraints, etc. At first
glance, the encoding looks full but it appears that when the lowest 3 bits are
Kind_Mem (meaning it's a memory constraint), the upper 16-bits are never
used. I've therefore used this gap to store a constraint ID which can then
be inspected during instruction selection.
Fantastic!  Yes, this is one of the places that I looked at, but to my 
great disappointment the connection with the original constraint was not 
preserved.

Thanks for doing this!

-Krzysztof

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
hosted by The Linux Foundation