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

s390x: Migrate branches and traps to ISLE #3724

Merged
merged 1 commit into from
Jan 31, 2022
Merged

s390x: Migrate branches and traps to ISLE #3724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
71 changes: 70 additions & 1 deletion cranelift/codegen/src/isa/s390x/inst.isle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,7 +679,6 @@
(type BoxCallInfo (primitive BoxCallInfo))
(type BoxCallIndInfo (primitive BoxCallIndInfo))
(type MachLabel (primitive MachLabel))
(type VecMachLabel (primitive VecMachLabel))
(type BoxJTSequenceInfo (primitive BoxJTSequenceInfo))
(type BoxExternalName (primitive BoxExternalName))
(type ValueLabel (primitive ValueLabel))
Expand Down Expand Up @@ -1040,6 +1039,19 @@
(extern extractor unsigned unsigned)


;; Helpers for machine label vectors ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

;; VecMachLabel needs to be passed by reference, so it cannot be
;; declared as primitive type. Declare as extern enum instead.
(type VecMachLabel extern (enum))

(decl vec_length_minus1 (VecMachLabel) u32)
(extern constructor vec_length_minus1 vec_length_minus1)

(decl vec_element (VecMachLabel u8) MachLabel)
(extern constructor vec_element vec_element)


;; Helpers for memory arguments ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

;; Accessors for `RelocDistance`.
Expand Down Expand Up @@ -1284,6 +1296,8 @@
;; implementation detail by helpers that preserve the SSA facade themselves.
(decl emit (MInst) Unit)
(extern constructor emit emit)
(decl emit_safepoint (MInst) Unit)
(extern constructor emit_safepoint emit_safepoint)
Comment on lines +1299 to +1300
Copy link
Member

Choose a reason for hiding this comment

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

And this should be in the prelude as well.

Copy link
Member

Choose a reason for hiding this comment

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

I see that emit actually isn't in the prelude, but we do use it from the prelude. So we should probably move emit to the prelude as well, since in practice it has to be the same declaration across all backends anyways. Happy to have that as part of this updated PR or we can do it in another PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's why I didn't put it in the prelude ...

I agree it makes sense to move emit / emit_safepoint there, but then we should also move the implementations to machinst/isle.re, right?

I'd be happy to do that, but I think it would be better to do it in another PR, as it will touch all back-ends.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we wouldn't necessarily have to move the implementations as well, nothing is stopping us from implementing the ISLE context's emit trait method once for each backend even when the decl is in the prelude.

In fact the x86_64 implementation is a little different, in that it calls a mov_mitosis method on each inst in emitted_insts, and if we made this code common then we would need to add such a method to the MachInst trait or something.

So I think we can move just the ISLE decl into the prelude without moving the rust trait implementations at all. Happy to have this in a follow up PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see. But even if we don't move the implementaton, once the decl is in the prelude as extern constructor, every back-end must implement it or it won't compile any more. So we'll still need to touch all back-ends. (In fact, with the x86 mov_mitosis implementation, I'm not sure exactly how the safepoint variant would work. I guess only the last instruction in the list should be the safepoint?)

Copy link
Member

Choose a reason for hiding this comment

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

once the decl is in the prelude as extern constructor, every back-end must implement it or it won't compile any more.

Correct. (Also, it is already the case that every backend must implement it or the ISLE won't compile because we are calling emit and assuming it is declared from inside the prelude already, it just so happens that we have N identical declarations for each backend instead of a single shared declaration in the prelude.)

(In fact, with the x86 mov_mitosis implementation, I'm not sure exactly how the safepoint variant would work. I guess only the last instruction in the list should be the safepoint?)

Yeah, I will have to think a bit about how to do this best. I think making only the last instruction a safepoint would work, but we might want to have some debug asserts in there too. Planning on tackling this after I'm done with my newtype wrappers PR by which time this PR will have merged.

Copy link
Member

Choose a reason for hiding this comment

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

I think making only the last instruction a safepoint would work,

Yeah, I think that's the correct solution -- the expanded list should always have the form of zero or more moves, followed by the original instruction; and the safepoint bit applies to the latter.

Copy link
Member

Choose a reason for hiding this comment

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

There are some instructions that always generate their results into a particular register, and so when our SSA-ified version of that instruction wants the result in a temp, we have to generate a move that follows the "real" instruction, so we can have trailing moves as well after move mitosis. This shouldn't be the case for safepoints I don't think (except maybe ABI/calling convention stuff where results are always in rax?) and we should be able to just debug assert this.


;; Helper for emitting `MInst.AluRRR` instructions.
(decl alu_rrr (Type ALUOp Reg Reg) Reg)
Expand Down Expand Up @@ -1695,6 +1709,26 @@
(_ Unit (emit (MInst.LoadAddr dst mem))))
(writable_reg_to_reg dst)))

;; Helper for emitting `MInst.Jump` instructions.
(decl jump_impl (MachLabel) SideEffectNoResult)
(rule (jump_impl target)
(SideEffectNoResult.Inst (MInst.Jump target)))

;; Helper for emitting `MInst.CondBr` instructions.
(decl cond_br (MachLabel MachLabel Cond) SideEffectNoResult)
(rule (cond_br taken not_taken cond)
(SideEffectNoResult.Inst (MInst.CondBr taken not_taken cond)))

;; Helper for emitting `MInst.OneWayCondBr` instructions.
(decl oneway_cond_br (MachLabel Cond) SideEffectNoResult)
(rule (oneway_cond_br dest cond)
(SideEffectNoResult.Inst (MInst.OneWayCondBr dest cond)))

;; Helper for emitting `MInst.JTSequence` instructions.
(decl jt_sequence (Reg VecMachLabel) SideEffectNoResult)
(rule (jt_sequence ridx targets)
(SideEffectNoResult.Inst (MInst.JTSequence ridx targets)))

;; Emit a `ProducesFlags` instruction when the flags are not actually needed.
(decl drop_flags (ProducesFlags) Reg)
(rule (drop_flags (ProducesFlags.ProducesFlags inst result))
Expand Down Expand Up @@ -2149,6 +2183,23 @@
src imm cond trap_code))))
(invalid_reg)))

(decl trap_impl (TrapCode) SideEffectNoResult)
(rule (trap_impl trap_code)
(SideEffectNoResult.Inst (MInst.Trap trap_code)))

(decl trap_if_impl (Cond TrapCode) SideEffectNoResult)
(rule (trap_if_impl cond trap_code)
(SideEffectNoResult.Inst (MInst.TrapIf cond trap_code)))

(decl debugtrap_impl () SideEffectNoResult)
(rule (debugtrap_impl)
(SideEffectNoResult.Inst (MInst.Debugtrap)))

(decl safepoint (SideEffectNoResult) ValueRegs)
(rule (safepoint (SideEffectNoResult.Inst inst))
(let ((_ Unit (emit_safepoint inst)))
(value_regs_invalid)))
Comment on lines +2198 to +2201
Copy link
Member

Choose a reason for hiding this comment

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

I think this should go in the shared prelude, next to value_regs_none.



;; Helpers for handling boolean conditions ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

Expand Down Expand Up @@ -2201,6 +2252,24 @@
(rule (lower_bool $B32 cond) (select_bool_imm $B32 cond -1 0))
(rule (lower_bool $B64 cond) (select_bool_imm $B64 cond -1 0))

;; Emit a conditional branch based on a boolean condition.
(decl cond_br_bool (ProducesBool MachLabel MachLabel) SideEffectNoResult)
(rule (cond_br_bool (ProducesBool.ProducesBool producer cond) taken not_taken)
(let ((_ Unit (emit_producer producer)))
(cond_br taken not_taken cond)))

;; Emit a one-way conditional branch based on a boolean condition.
(decl oneway_cond_br_bool (ProducesBool MachLabel) SideEffectNoResult)
(rule (oneway_cond_br_bool (ProducesBool.ProducesBool producer cond) dest)
(let ((_ Unit (emit_producer producer)))
(oneway_cond_br dest cond)))

;; Emit a conditional trap based on a boolean condition.
(decl trap_if_bool (ProducesBool TrapCode) SideEffectNoResult)
(rule (trap_if_bool (ProducesBool.ProducesBool producer cond) trap_code)
(let ((_ Unit (emit_producer producer)))
(trap_if_impl cond trap_code)))


;; Helpers for generating `clz` instructions ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

Expand Down
153 changes: 153 additions & 0 deletions cranelift/codegen/src/isa/s390x/lower.isle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@
;; register(s) within which the lowered instruction's result values live.
(decl lower (Inst) ValueRegs)

;; A variant of the main lowering constructor term, used for branches.
;; The only difference is that it gets an extra argument holding a vector
;; of branch targets to be used.
(decl lower_branch (Inst VecMachLabel) ValueRegs)


;;;; Rules for `iconst` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

Expand Down Expand Up @@ -1778,3 +1783,151 @@
(value_reg (select_bool_reg ty (icmp_val $false int_cc x y)
(put_in_reg val_true) (put_in_reg val_false))))


;;;; Rules for `jump` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

;; Unconditional branch. The target is found as first (and only) element in
;; the list of the current block's branch targets passed as `targets`.
(rule (lower_branch (jump _ _) targets)
(value_regs_none (jump_impl (vec_element targets 0))))


;;;; Rules for `br_table` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

;; Jump table. `targets` contains the default target followed by the
;; list of branch targets per index value.
(rule (lower_branch (br_table val_idx _ _) targets)
(let ((idx Reg (put_in_reg_zext64 val_idx))
;; Bounds-check the index and branch to default.
;; This is an internal branch that is not a terminator insn.
;; Instead, the default target is listed a potential target
;; in the final JTSequence, which is the block terminator.
(cond ProducesBool
(bool (icmpu_uimm32 $I64 idx (vec_length_minus1 targets))
(intcc_as_cond (IntCC.UnsignedGreaterThanOrEqual))))
(_ ValueRegs (value_regs_none (oneway_cond_br_bool cond
(vec_element targets 0)))))
;; Scale the index by the element size, and then emit the
;; compound instruction that does:
;;
;; larl %r1, <jt-base>
;; agf %r1, 0(%r1, %rScaledIndex)
;; br %r1
;; [jt entries]
;;
;; This must be *one* instruction in the vcode because
;; we cannot allow regalloc to insert any spills/fills
;; in the middle of the sequence; otherwise, the LARL's
;; PC-rel offset to the jumptable would be incorrect.
;; (The alternative is to introduce a relocation pass
;; for inlined jumptables, which is much worse, IMHO.)
(value_regs_none (jt_sequence (lshl_imm $I64 idx 2) targets))))


;;;; Rules for `brz` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

;; Two-way conditional branch on zero. `targets` contains:
;; - element 0: target if the condition is true (i.e. value is zero)
;; - element 1: target if the condition is false (i.e. value is nonzero)
(rule (lower_branch (brz val_cond _ _) targets)
(value_regs_none (cond_br_bool (invert_bool (value_nonzero val_cond))
(vec_element targets 0)
(vec_element targets 1))))


;;;; Rules for `brnz` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

;; Two-way conditional branch on nonzero. `targets` contains:
;; - element 0: target if the condition is true (i.e. value is nonzero)
;; - element 1: target if the condition is false (i.e. value is zero)
(rule (lower_branch (brnz val_cond _ _) targets)
(value_regs_none (cond_br_bool (value_nonzero val_cond)
(vec_element targets 0)
(vec_element targets 1))))


;;;; Rules for `brif` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

;; Similarly to `selectif_spectre_guard`, we only recognize specific patterns
;; generated by common code here. Others will fail to lower.

(rule (lower_branch (brif int_cc (def_inst (ifcmp x y)) _ _) targets)
(value_regs_none (cond_br_bool (icmp_val $false int_cc x y)
(vec_element targets 0)
(vec_element targets 1))))


;;;; Rules for `trap` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

(rule (lower (trap trap_code))
(safepoint (trap_impl trap_code)))


;;;; Rules for `resumable_trap` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

(rule (lower (resumable_trap trap_code))
(safepoint (trap_impl trap_code)))


;;;; Rules for `trapz` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

(rule (lower (trapz val trap_code))
(safepoint (trap_if_bool (invert_bool (value_nonzero val)) trap_code)))


;;;; Rules for `trapnz` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

(rule (lower (trapnz val trap_code))
(safepoint (trap_if_bool (value_nonzero val) trap_code)))


;;;; Rules for `resumable_trapnz` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

(rule (lower (resumable_trapnz val trap_code))
(safepoint (trap_if_bool (value_nonzero val) trap_code)))


;;;; Rules for `debugtrap` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

(rule (lower (debugtrap))
(value_regs_none (debugtrap_impl)))


;;;; Rules for `trapif` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

;; Similarly to `selectif_spectre_guard`, we only recognize specific patterns
;; generated by common code here. Others will fail to lower.

;; Recognize the case of `ifcmp` feeding into `trapif`. Directly generate
;; the desired comparison here; there is no separate `ifcmp` lowering.

(rule (lower (trapif int_cc (def_inst (ifcmp x y)) trap_code))
(safepoint (trap_if_bool (icmp_val $false int_cc x y) trap_code)))

;; Recognize the case of `iadd_ifcout` feeding into `trapif`. Note that
;; in the case, the `iadd_ifcout` is generated by a separate lowering
;; (in order to properly handle the register output of that instruction.)
;;
;; The flags must not have been clobbered by any other instruction between the
;; iadd_ifcout and this instruction, as verified by the CLIF validator; so we
;; can simply rely on the condition code here.
;;
;; IaddIfcout is implemented via a ADD LOGICAL instruction, which sets the
;; the condition code as follows:
;; 0 Result zero; no carry
;; 1 Result not zero; no carry
;; 2 Result zero; carry
;; 3 Result not zero; carry
;; This means "carry" corresponds to condition code 2 or 3, i.e.
;; a condition mask of 2 | 1.
;;
;; As this does not match any of the encodings used with a normal integer
;; comparsion, this cannot be represented by any IntCC value. We need to
;; remap the IntCC::UnsignedGreaterThan value that we have here as result
;; of the unsigned_add_overflow_condition call to the correct mask.

(rule (lower (trapif (IntCC.UnsignedGreaterThan)
(def_inst (iadd_ifcout x y)) trap_code))
(value_regs_none (trap_if_impl (mask_as_cond 3) trap_code)))


Loading