[PATCH] starting from ARMv8, rGPR includes SP

[Artyom Skrobov]

John, thank you for your investigation!

I suspect that making ARMMCRegisterClasses mutable would just open a pandora
box: LLVM is supposed to be embeddable in a host process, and so all kinds
of weirdness will ensue when one client thread tries to compile for ARMv7
and another thread for ARMv8 at the same time.

Regarding creating an alternative version of ARMMCRegisterClasses: I'm not
sure how this would be possible, other than by including
ARMGenRegisterInfo.inc for a second time, with a similarly nasty set of
redefinitions, as in my patch.
Even if so, this would be harder than creating an alternative version of
ARMInsts, as my patch does -- because ARMGenRegisterInfo.inc also contains
definitions for many register subclasses (such as
GPRPair_with_gsub_1_in_hGPR_and_rGPRRegClassID) and subclass relationships,
all of which would need redefinition.


Evan, you're listed in CODE_OWNERS.TXT for the ARM backend -- would you care
to take a look at these patches?



-----Original Message-----
From: John Brawn 
Sent: 29 June 2015 13:55
To: Artyom Skrobov; llvm-commits at cs.uiuc.edu
Subject: RE: [PATCH] starting from ARMv8, rGPR includes SP

> John, at first I thought the same; but if I just add SP to rGPR,
> test/CodeGen/Thumb2/2010-04-15-DynAllocBug.ll shows that alloca gets
lowered
> into "bic sp, r0, #7", which is unpredictable on ARMv7.
> This is because the reservedness of SP doesn't affect the constructions
> which are already tied to the stack pointer.

Yes, it looks like MachineCopyPropagation is eliminating a copy because it
sees that
SP is in the register class of the target register. 

Hmm. I'm not sure what the best solution is then. It looks like the
fundamental issue
is that register classes are const and stored in a big const MCRegisterClass
array. Maybe
that's what needs to change? Or maybe we should somehow create an alternate
version
of ARMMCRegisterClasses and somehow select between the two.

I have no idea, I'm just guessing here.

John

> -----Original Message-----
> From: Artyom Skrobov
> Sent: 23 June 2015 14:04
> To: John Brawn; llvm-commits at cs.uiuc.edu
> Subject: RE: [PATCH] starting from ARMv8, rGPR includes SP
> 
> John, at first I thought the same; but if I just add SP to rGPR,
> test/CodeGen/Thumb2/2010-04-15-DynAllocBug.ll shows that alloca gets
lowered
> into "bic sp, r0, #7", which is unpredictable on ARMv7.
> This is because the reservedness of SP doesn't affect the constructions
> which are already tied to the stack pointer.
> 
> On the other hand, making SP an invalid operand for t2BICri causes the
> alloca to get lowered into "bic r0, r0, #7; mov sp, r0" instead.
> 
> 
> -----Original Message-----
> From: John Brawn
> Sent: 23 June 2015 12:13
> To: Artyom Skrobov; llvm-commits at cs.uiuc.edu
> Subject: RE: [PATCH] starting from ARMv8, rGPR includes SP
> 
> > Is it not possible to do this by actually adding SP to rGPR in the
> > RegisterClass definition? There
> > would have to be some logic somewhere that then allows it or not based
on
> > whether the
> > subtarget has ARMv8, but I don't know where that would be so maybe that
> > approach wouldn't
> > work.
> 
> I've looked at this a little and I think the closest equivalent to this is
> the handling of hasD16,
> which is handled by:
>  * ARMBaseRegisterInfo::getReservedRegs marks D17-D31 as reserved
>  * ARMAsmParser::tryParseRegister rejects D17-D31
>  * DecodeDPRRegisterClass in ARMDisassembler.cpp rejects D17-D31
> 
> I think if you just add SP to rGPR then actually everything should just
> work: SP is already
> marked as reserved, and it looks like you already have the necessary
changes
> to parse and
> decode.
> 
> John
> 
> > -----Original Message-----
> > From: llvm-commits-bounces at cs.uiuc.edu
> [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of John Brawn
> > Sent: 22 June 2015 18:24
> > To: Artyom Skrobov; llvm-commits at cs.uiuc.edu
> > Subject: RE: [PATCH] starting from ARMv8, rGPR includes SP
> >
> > This isn't "rGPR includes SP" it's "rGPR is #defined to GPRnopc", which
> > makes things really
> > confusing if you're just looking at the .td files to figure out what's
> what
> > - you see instructions
> > that use rGPR, and you see a definition of the rGPR register class, but
> > those instructions may
> > actually be redefined to use GPRnopc due to something happening
elsewhere
> > that you
> > haven't noticed.
> >
> > Is it not possible to do this by actually adding SP to rGPR in the
> > RegisterClass definition? There
> > would have to be some logic somewhere that then allows it or not based
on
> > whether the
> > subtarget has ARMv8, but I don't know where that would be so maybe that
> > approach wouldn't
> > work.
> >
> > John
> >
> >
> > > -----Original Message-----
> > > From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-
> > > bounces at cs.uiuc.edu] On Behalf Of Artyom Skrobov
> > > Sent: 22 June 2015 10:08
> > > To: llvm-commits at cs.uiuc.edu
> > > Subject: [PATCH] starting from ARMv8, rGPR includes SP
> > >
> > > Hello,
> > >
> > > A set of six patches, more or less independent one from another, are
> > > attached for review.
> > >
> > > The people who were last to contribute to the code that my patches are
> > > changing are on CC:
> > >
> > > The first patch fixes a long-standing error in the assembler which
> allowed
> > > SP and PC as shifted-reg operands in ARMv7 Thumb -- despite correctly
> > > rejecting the same registers when not shifted.
> > > A test case which appeared to enshrine this incorrect treatment,
> > > test/MC/ARM/thumb-shift-encoding.s, was committed by Tim back in 2012;
> > > my
> > > patch fixes his test, and moves the invalid instructions into
> > > test/MC/ARM/thumb2-diagnostics.s
> > >
> > > The second patch adds a case for MCK_rGPR into
> > > ARMAsmParser::validateTargetOperandClass, to allow SP in rGPR, when
> > > assembling for ARMv8 Thumb.
> > > The third patch, similarly, extends DecoderGPRRegisterClass to allow
SP
> in
> > > rGPR, when disassembling ARMv8 Thumb; and changes
> > > DecodeSORegImmOperand to
> > > disallow PC (and optionally SP) even when shifted.
> > > The existing implementation of DecodeSORegImmOperand was committed
> > > by Owen
> > > in 2011; and of DecoderGPRRegisterClass, by Amaury in 2013.
> > >
> > > The next patch includes ARMGenInstrInfo.inc for a second time, after
> > > re-defining rGPRRegClassID, in order to enable the codegen to use the
> new
> > > encodings that become available in ARMv8 Thumb.
> > > This patch includes a small tweak to InstrInfoEmitter, so that
> > > ARMInstrNameData and ARMInstrNameIndices don't get re-defined.
> > > It also requires to make the getXxxDeprecationInfo predicate functions
> > > non-static.
> > > The code changed by this patch was committed by Tim in 2013.
> > >
> > > The last two patches are to revert the special case for BXJ,
> conditionally
> > > accepting either a GPRnopc or a rGRP operand; and to make
> > > test/MC/ARM/diagnostics.s and test/MC/ARM/thumb2-diagnostics.s more
> > > encompassing, by checking both ARMv7 and ARMv8 handling.
> > > The BXJ patch was committed by Charlie, after review by Renato, just
two
> > > month ago.
> > >
> > > Any comments on these patches?
> >
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> 
> 
>