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.

Sign up for GitHub

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

[SOL] Remove neg and change the semantics of sub #73

Merged
merged 4 commits into from
Dec 16, 2023
Merged

[SOL] Remove neg and change the semantics of sub #73

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ee22760
Remove neg and change the semantics of sub
LucasSte Dec 12, 2023
5b91bed
Reorganize tests
LucasSte Dec 13, 2023
30f98d9
Reorganize instruction declarations
LucasSte Dec 14, 2023
8497ade
Start feature flag
LucasSte Dec 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 34 additions & 6 deletions llvm/lib/Target/SBF/SBFInstrInfo.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ def SBFIsBigEndian : Predicate<"!CurDAG->getDataLayout().isLittleEndian()">;
def SBFHasALU32 : Predicate<"Subtarget->getHasAlu32()">;
def SBFNoALU32 : Predicate<"!Subtarget->getHasAlu32()">;
def SBFSubtargetSolana : Predicate<"Subtarget->isSolana()">;
def SBFv2 : Predicate<"Subtarget->isSBFv2()">;
def NoSBFv2 : Predicate<"!Subtarget->isSBFv2()">;

def brtarget : Operand<OtherVT> {
let PrintMethod = "printBrTargetOperand";
Expand Down Expand Up @@ -263,7 +265,7 @@ class ALU_RR<SBFOpClass Class, SBFArithOp Opc,
let SBFClass = Class;
}

multiclass ALU<SBFArithOp Opc, string Mnemonic, SDNode OpNode> {
multiclass ALU<SBFArithOp Opc, string Mnemonic, SDNode OpNode, bit UseImmPat = 1> {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LLVM Coding standard nit: (80-column rules) Please move the new parameter to the next line, aligned with the <'.

def _rr : ALU_RR<SBF_ALU64, Opc,
(outs GPR:$dst),
(ins GPR:$src2, GPR:$src),
Expand All @@ -273,7 +275,9 @@ multiclass ALU<SBFArithOp Opc, string Mnemonic, SDNode OpNode> {
(outs GPR:$dst),
(ins GPR:$src2, i64imm:$imm),
Mnemonic # "64 $dst, $imm",
[(set GPR:$dst, (OpNode GPR:$src2, i64immSExt32:$imm))]>;
!if(UseImmPat,
[(set GPR:$dst,
(OpNode GPR:$src2, i64immSExt32:$imm))], [])>;
def _rr_32 : ALU_RR<SBF_ALU, Opc,
(outs GPR32:$dst),
(ins GPR32:$src2, GPR32:$src),
Expand All @@ -283,13 +287,15 @@ multiclass ALU<SBFArithOp Opc, string Mnemonic, SDNode OpNode> {
(outs GPR32:$dst),
(ins GPR32:$src2, i32imm:$imm),
Mnemonic # "32 $dst, $imm",
[(set GPR32:$dst, (OpNode GPR32:$src2, i32immSExt32:$imm))]>;
!if(UseImmPat,
[(set GPR32:$dst,
(OpNode GPR32:$src2, i32immSExt32:$imm))], [])>;
}

let Constraints = "$dst = $src2" in {
let isAsCheapAsAMove = 1 in {
defm ADD : ALU<SBF_ADD, "add", add>;
defm SUB : ALU<SBF_SUB, "sub", sub>;
defm SUB : ALU<SBF_SUB, "sub", sub, 0>;
defm OR : ALU<SBF_OR, "or", or>;
defm AND : ALU<SBF_AND, "and", and>;
defm SLL : ALU<SBF_LSH, "lsh", shl>;
Expand All @@ -306,6 +312,19 @@ let Constraints = "$dst = $src2" in {
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The entire part of the patch above involving the ALU multiclass and the SUB definition need to be reworked and simplified. That is, there is no need to refactor and duplicate the multiclass. We can succinctly adjust the original multiclass by conditionally swapping the pattern operands as follows:

@@ -264,6 +264,12 @@ class ALU_RR<SBFOpClass Class, SBFArithOp Opc,
 }
 
 multiclass ALU<SBFArithOp Opc, string Mnemonic, SDNode OpNode> {
+  // Match swapped operand order only for special case of 'sub reg,imm`.
+  defvar DoSwap = !eq(OpNode, sub);
+  defvar RegImmPat = !if(DoSwap, (OpNode i64immSExt32:$imm, GPR:$src2),
+                                 (OpNode GPR:$src2, i64immSExt32:$imm));
+  defvar RegImmPat32 = !if(DoSwap,(OpNode i32immSExt32:$imm, GPR32:$src2),
+                                  (OpNode GPR32:$src2, i32immSExt32:$imm));
   def _rr : ALU_RR<SBF_ALU64, Opc,
                    (outs GPR:$dst),
                    (ins GPR:$src2, GPR:$src),
@@ -273,7 +279,7 @@ multiclass ALU<SBFArithOp Opc, string Mnemonic, SDNode OpNod
e> {
                    (outs GPR:$dst),
                    (ins GPR:$src2, i64imm:$imm),
                    Mnemonic # "64 $dst, $imm",
-                   [(set GPR:$dst, (OpNode GPR:$src2, i64immSExt32:$imm))]>;
+                   [(set GPR:$dst, RegImmPat)]>;
   def _rr_32 : ALU_RR<SBF_ALU, Opc,
                    (outs GPR32:$dst),
                    (ins GPR32:$src2, GPR32:$src),
@@ -283,7 +289,7 @@ multiclass ALU<SBFArithOp Opc, string Mnemonic, SDNode OpNod
e> {
                    (outs GPR32:$dst),
                    (ins GPR32:$src2, i32imm:$imm),
                    Mnemonic # "32 $dst, $imm",
-                   [(set GPR32:$dst, (OpNode GPR32:$src2, i32immSExt32:$imm))]>
;
+                   [(set GPR32:$dst, RegImmPat32)]>;
 }
 
 let Constraints = "$dst = $src2" in {
@@ -306,24 +312,6 @@ let Constraints = "$dst = $src2" in {
     }
 }

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One related general question I have is-- should we conditionally be generating the swapped special case sub? We need to exactly match the runtime semantics on chain today and down the road.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new syntax can be behind the sbfv2 cpu perhaps?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find a way to encapsulate the new sub inside a predicate for sbfv2. All the other example cases do not redefine the same instruction. Instead, the predicate encapsulate definitions that only exist for a certain feature and do not override other definitions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything should be behind a feature flag now.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for incorporating my offline suggestions, this is ready to land (other than some trivial coding standard nits, which I'll note shortly).


// Special case for SBFv2
// In SBFv1, `sub reg, imm` is interpreted as reg = reg - imm,
// but in SBFv2 it means reg = imm - reg
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LLVM Coding standard nit: (Proper prose, capitalization, punctuation) Please add period/full-stops (as in the original comments that I had written-- but your wording is fine).

+// Special case cpu wart:
+// Operand order for 'sub reg,imm' is interpreted
+// differently for pre-sbfv2 cpu versus sbfv2. In the former case, the
+// VM executes 'reg-imm'. For the latter, 'imm-reg'.
+// Define two sets of 'sub' patterns accordingly.

def : Pat<(sub GPR:$src, i64immSExt32:$imm),
(SUB_ri GPR:$src, i64immSExt32:$imm)>, Requires<[NoSBFv2]>;
def : Pat<(sub GPR32:$src, i32immSExt32:$imm),
(SUB_ri_32 GPR32:$src, i32immSExt32:$imm)>, Requires<[NoSBFv2]>;

def : Pat<(sub i64immSExt32:$imm, GPR:$src),
(SUB_ri GPR:$src, i64immSExt32:$imm)>, Requires<[SBFv2]>;
def : Pat<(sub i32immSExt32:$imm, GPR32:$src),
(SUB_ri_32 GPR32:$src, i32immSExt32:$imm)>, Requires<[SBFv2]>;

class NEG_RR<SBFOpClass Class, SBFArithOp Opc,
dag outs, dag ins, string asmstr, list<dag> pattern>
: TYPE_ALU_JMP<Opc.Value, 0, outs, ins, asmstr, pattern> {
Expand All @@ -318,12 +337,21 @@ class NEG_RR<SBFOpClass Class, SBFArithOp Opc,
let Constraints = "$dst = $src", isAsCheapAsAMove = 1 in {
def NEG_64: NEG_RR<SBF_ALU64, SBF_NEG, (outs GPR:$dst), (ins GPR:$src),
"neg64 $dst",
[(set GPR:$dst, (ineg i64:$src))]>;
[]>;
def NEG_32: NEG_RR<SBF_ALU, SBF_NEG, (outs GPR32:$dst), (ins GPR32:$src),
"neg32 $dst",
[(set GPR32:$dst, (ineg i32:$src))]>;
[]>;
}

// Instruction `neg` exists on SBFv1, but not on SBFv2
// In SBFv2, the negate operation is done with a subtraction
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto: Periods/full-stops.

def : Pat<(ineg i64:$src), (NEG_64 GPR:$src)>, Requires<[NoSBFv2]>;
def : Pat<(ineg i32:$src), (NEG_32 GPR32:$src)>, Requires<[NoSBFv2]>;

def : Pat<(ineg i64:$src), (SUB_ri GPR:$src, 0)>, Requires<[SBFv2]>;
def : Pat<(ineg i32:$src), (SUB_ri_32 GPR32:$src, 0)>, Requires<[SBFv2]>;


class LD_IMM64<bits<4> Pseudo, string Mnemonic>
: TYPE_LD_ST<SBF_IMM.Value, SBF_DW.Value,
(outs GPR:$dst),
Expand Down
7 changes: 5 additions & 2 deletions llvm/lib/Target/SBF/SBFSubtarget.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ void SBFSubtarget::initializeEnvironment(const Triple &TT) {
HasDynamicFrames = false;
HasSdiv = false;
UseDwarfRIS = false;
IsSBFv2 = false;
}

void SBFSubtarget::initSubtargetFeatures(StringRef CPU, StringRef FS) {
Expand All @@ -64,8 +65,10 @@ void SBFSubtarget::initSubtargetFeatures(StringRef CPU, StringRef FS) {
HasAlu32 = true;
}

if (CPU == "sbfv2" && !HasDynamicFrames) {
report_fatal_error("sbfv2 requires dynamic-frames\n", false);
if (CPU == "sbfv2") {
IsSBFv2 = true;
if (!HasDynamicFrames)
report_fatal_error("sbfv2 requires dynamic-frames\n", false);
}
}

Expand Down
9 changes: 7 additions & 2 deletions llvm/lib/Target/SBF/SBFSubtarget.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,17 @@ class SBFSubtarget : public SBFGenSubtargetInfo {
// whether we should enable MCAsmInfo DwarfUsesRelocationsAcrossSections
bool UseDwarfRIS;

// whether we are targeting SBFv2
bool IsSBFv2;

public:
// This constructor initializes the data members to match that
// of the specified triple.
SBFSubtarget(const Triple &TT, const std::string &CPU, const std::string &FS,
const TargetMachine &TM);

SBFSubtarget &initializeSubtargetDependencies(const Triple &TT, StringRef CPU, StringRef FS);
SBFSubtarget &initializeSubtargetDependencies(const Triple &TT, StringRef CPU,
StringRef FS);

// ParseSubtargetFeatures - Parses features string setting specified
// subtarget options. Definition of function is auto generated by tblgen.
Expand All @@ -90,6 +94,7 @@ class SBFSubtarget : public SBFGenSubtargetInfo {
bool getHasDynamicFrames() const { return HasDynamicFrames; }
bool getHasSdiv() const { return HasSdiv; }
bool getUseDwarfRIS() const { return UseDwarfRIS; }
bool isSBFv2() const { return IsSBFv2; }

const SBFInstrInfo *getInstrInfo() const override { return &InstrInfo; }
const SBFFrameLowering *getFrameLowering() const override {
Expand All @@ -105,6 +110,6 @@ class SBFSubtarget : public SBFGenSubtargetInfo {
return &InstrInfo.getRegisterInfo();
}
};
} // End llvm namespace
} // namespace llvm

#endif
79 changes: 79 additions & 0 deletions llvm/test/CodeGen/SBF/sub_reversed_immediate.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
; RUN: llc < %s -march=sbf -mattr=+alu32 | FileCheck --check-prefix=CHECK-v1 %s
; RUN: llc < %s -march=sbf -mattr=+alu32 -mcpu=sbfv2 | FileCheck --check-prefix=CHECK-v2 %s


; Function Attrs: norecurse nounwind readnone
define dso_local i64 @sub_imm_minus_reg_64(i64 %a) #0 {
entry:
; CHECK-LABEL: sub_imm_minus_reg_64:
%sub = sub nsw i64 50, %a

; CHECK-v1: mov64 r{{[0-9]+}}, 50
; CHECK-v1: sub64 r{{[0-9]+}}, r{{[0-9]+}}

; CHECK-v2: sub64 r{{[0-9]+}}, 50
ret i64 %sub
}

; Function Attrs: norecurse nounwind readnone
define dso_local i64 @sub_reg_minus_imm_64(i64 %a) #0 {
entry:
; CHECK-LABEL: sub_reg_minus_imm_64:
%t = sub nsw i64 %a, 50
; CHECK-v1: add64 r{{[0-9]+}}, -50

; CHECK-v2: add64 r{{[0-9]+}}, -50
ret i64 %t
}


; Function Attrs: norecurse nounwind readnone
define dso_local i32 @sub_imm_minus_reg_32(i32 %a) #0 {
entry:
; CHECK-LABEL: sub_imm_minus_reg_32:
%sub = sub nsw i32 50, %a

; CHECK-v1: mov32 w{{[0-9]+}}, 50
; CHECK-v1: sub32 w{{[0-9]+}}, w{{[0-9]+}}

; CHECK-v2: sub32 w{{[0-9]+}}, 50

ret i32 %sub
}

; Function Attrs: norecurse nounwind readnone
define dso_local i32 @sub_reg_minus_imm_32(i32 %a) #0 {
entry:
; CHECK-LABEL: sub_reg_minus_imm_32:
%t = sub nsw i32 %a, 50

; CHECK-v1: add32 w{{[0-9]+}}, -50

; CHECK-v2: add32 w{{[0-9]+}}, -50
ret i32 %t
}


; Function Attrs: norecurse nounwind readnone
define dso_local i64 @neg_64(i64 %a) #0 {
entry:
; CHECK-LABEL: neg_64:
%sub = sub nsw i64 0, %a

; CHECK-v1: neg64 r{{[0-9]+}}
; CHECK-v2: sub64 r{{[0-9]+}}, 0

ret i64 %sub
}

; Function Attrs: norecurse nounwind readnone
define dso_local i32 @neg_32(i32 %a) #0 {
entry:
; CHECK-LABEL: neg_32:
%sub = sub nsw i32 0, %a

; CHECK-v1: neg32 w{{[0-9]+}}
; CHECK-v2: sub32 w{{[0-9]+}}, 0

ret i32 %sub
}
Loading