Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARM backend incorrectly limits smlal and umlal instructions to amv6 #18021

Closed
llvmbot opened this issue Oct 22, 2013 · 4 comments
Closed

ARM backend incorrectly limits smlal and umlal instructions to amv6 #18021

llvmbot opened this issue Oct 22, 2013 · 4 comments
Labels
bugzilla Issues migrated from bugzilla tools:llc

Comments

@llvmbot
Copy link
Collaborator

llvmbot commented Oct 22, 2013

Bugzilla Link 17647
Resolution FIXED
Resolved on Feb 21, 2014 05:20
Version trunk
OS Linux
Blocks #19300
Reporter LLVM Bugzilla Contributor
CC @rengolin

Extended Description

If I try to compile a simple test that uses inline asm to generate the smlal and umlal instructions clang fails with an error like this:

$ cat smlal.c
void sfoo(signed a, signed b) {
signed lo;
signed hi;
asm volatile ("smlal %0,%1,%2,%3" : "+&r" (lo), "+&r"(hi) : "r" (a), "r" (b));
}

void ufoo(unsigned a, unsigned b) {
unsigned lo;
unsigned hi;
asm volatile ("umlal %0,%1,%2,%3" : "+&r" (lo), "+&r"(hi) : "r" (a), "r" (b));
}

$ clang -O -march=armv4 smlal.c -c
smlal.c:5:17: error: instruction requires: armv6
asm volatile ("smlal %0,%1,%2,%3" : "+&r" (lo), "+&r"(hi) : "r" (a), "r" (b) : "cc");
^
:1:2: note: instantiated into assembly here
smlal r2,r3,r0,r1
^
smlal.c:11:17: error: instruction requires: armv6
asm volatile ("umlal %0,%1,%2,%3" : "+&r" (lo), "+&r"(hi) : "r" (a), "r" (b) : "cc");
^
:1:2: note: instantiated into assembly here
umlal r2,r3,r0,r1

If I compile with gcc or disable the integrated assembler (using -no-integrated-as), it works fine. Looking at the definition for these instructions in ARMInstrInfo.td I see this:

def SMLAL : AsMla1I64<0b0000111, (outs GPR:$RdLo, GPR:$RdHi),
(ins GPR:$Rn, GPR:$Rm, GPR:$RLo, GPR:$RHi), IIC_iMAC64,
"smlal", "\t$RdLo, $RdHi, $Rn, $Rm", []>,
RegConstraint<"$RLo = $RdLo, $RHi = $RdHi">, Requires<[IsARM, HasV6]>;
def UMLAL : AsMla1I64<0b0000101, (outs GPR:$RdLo, GPR:$RdHi),
(ins GPR:$Rn, GPR:$Rm, GPR:$RLo, GPR:$RHi), IIC_iMAC64,
"umlal", "\t$RdLo, $RdHi, $Rn, $Rm", []>,
RegConstraint<"$RLo = $RdLo, $RHi = $RdHi">, Requires<[IsARM, HasV6]>;

Both instructions are defined with Requires<[HasV6]>, but the ARM ARM (A8.8.178) says that these instructions are available in armv4* and above (encoding A1).

I also see these definitions in the same file:

def SMLALv5 : ARMPseudoExpand<(outs GPR:$RdLo, GPR:$RdHi),
(ins GPR:$Rn, GPR:$Rm, GPR:$RLo, GPR:$RHi, pred:$p, cc_out:$s),
4, IIC_iMAC64, [],
(SMLAL GPR:$RdLo, GPR:$RdHi, GPR:$Rn, GPR:$Rm, GPR:$RLo, GPR:$RHi,
pred:$p, cc_out:$s)>,
Requires<[IsARM, NoV6]>;
def UMLALv5 : ARMPseudoExpand<(outs GPR:$RdLo, GPR:$RdHi),
(ins GPR:$Rn, GPR:$Rm, GPR:$RLo, GPR:$RHi, pred:$p, cc_out:$s),
4, IIC_iMAC64, [],
(UMLAL GPR:$RdLo, GPR:$RdHi, GPR:$Rn, GPR:$Rm, GPR:$RLo, GPR:$RHi,
pred:$p, cc_out:$s)>,
Requires<[IsARM, NoV6]>;

I am not sure why these are defined separately for non-v6 targets and what they are trying to model.

@llvmbot
Copy link
Collaborator Author

llvmbot commented Dec 21, 2013

Assuming there's a valid reason so that the v5 version should be defined separately, should the DAG selector select them rather than the v6 version when using armv4/armv5?

@llvmbot
Copy link
Collaborator Author

llvmbot commented Jan 4, 2014

I am not sure why these are defined separately for non-v6 targets and what
they are trying to model.

From ARM architecture reference manual, we found that v5 (and below) arches have additional constraints on these instructions:

if ArchVersion() < 6 && (dHi == n || dLo == n) then UNPREDICTABLE;

DAGISel relies on the v5 definitions for generating correct code. However, this mechanism renders such instructions unavailable to the IAS.
Other instructions facing same situation: MUL, MLA, SMULL and UMULL.

There's already a patch enabling SMULL/UMULL to IAS by defining InstAlias. We can do the same for MUL/MLA/SMLAL/UMLAL.

Going through the .td file, I found two questions:

  1. Register classes are inconsistent.
    MUL/MLA/SMULL/UMULL/SMLAL/UMLAL all require operands NOT to be $pc:
    if dLo == 15 || dHi == 15 || n == 15 || m == 15 then UNPREDICTABLE

But only MUL is defined using class GPRnopc:
let isCommutable = 1, TwoOperandAliasConstraint = "$Rn = $Rd" in {
def MUL : AsMul1I32<0b0000000, (outs GPRnopc:$Rd),
(ins GPRnopc:$Rn, GPRnopc:$Rm),
IIC_iMUL32, "mul", "\t$Rd, $Rn, $Rm",
[(set GPRnopc:$Rd, (mul GPRnopc:$Rn, GPRnopc:$Rm))]>,
Requires<[IsARM, HasV6]> {
let Inst{15-12} = 0b0000;
let Unpredictable{15-12} = 0b1111;
}
let Constraints = "@earlyclobber $Rd" in
def MULv5: ARMPseudoExpand<(outs GPRnopc:$Rd), (ins GPRnopc:$Rn, GPRnopc:$Rm,
pred:$p, cc_out:$s),
4,IIC_iMUL32,
[(set GPRnopc:$Rd, (mul GPRnopc:$Rn,GPRnopc:$Rm))],
(MUL GPRnopc:$Rd, GPRnopc:$Rn, GPRnopc:$Rm, pred:$p, cc_out:$s)>,
Requires<[IsARM, NoV6,UseMulOps]>;
}

def MLA : AsMul1I32<0b0000001, (outs GPR:$Rd), (ins GPR:$Rn, GPR:$Rm, GPR:$Ra),
IIC_iMAC32, "mla", "\t$Rd, $Rn, $Rm, $Ra",
[(set GPR:$Rd, (add (mul GPR:$Rn, GPR:$Rm), GPR:$Ra))]>,
Requires<[IsARM, HasV6, UseMulOps]> {
bits<4> Ra;
let Inst{15-12} = Ra;
}

let Constraints = "@earlyclobber $Rd" in
def MLAv5: ARMPseudoExpand<(outs GPR:$Rd),
(ins GPR:$Rn, GPR:$Rm, GPR:$Ra, pred:$p, cc_out:$s),
4, IIC_iMAC32,
[(set GPR:$Rd, (add (mul GPR:$Rn, GPR:$Rm), GPR:$Ra))],
(MLA GPR:$Rd, GPR:$Rn, GPR:$Rm, GPR:$Ra, pred:$p, cc_out:$s)>,
Requires<[IsARM, NoV6]>;

  1. I don't see UMAAL is available for v5 in the manual but it has a v5 pseudo defined:
    let Constraints = "@earlyclobber $RdLo,@earlyclobber $RdHi" in {
    def UMAALv5 : ARMPseudoExpand<(outs GPR:$RdLo, GPR:$RdHi),
    (ins GPR:$Rn, GPR:$Rm, pred:$p),
    4, IIC_iMAC64, [],
    (UMAAL GPR:$RdLo, GPR:$RdHi, GPR:$Rn, GPR:$Rm, pred:$p)>,
    Requires<[IsARM, NoV6]>;
    }

@llvmbot
Copy link
Collaborator Author

llvmbot commented Jan 18, 2014

r199026: adds MUL/SMLAL/UMLAL aliases for ARMv4.
r199211: removes unused UMAALv5 def.
r199491: adds MLA alias for ARMv4, makes MLA defs use register class GPRnopc.

@rengolin
Copy link
Member

mentioned in issue #19300

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 9, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla tools:llc
Projects
None yet
Development

No branches or pull requests

2 participants