From 71c568b7df68e73a2c9475ed1e66f9f0e129364e Mon Sep 17 00:00:00 2001 From: Vincent Laviron Date: Mon, 12 Dec 2022 11:22:47 +0100 Subject: [PATCH 1/3] x86: Force result of Icomp to be in a register --- asmcomp/amd64/reload.ml | 21 +++++++++++++++++++-- asmcomp/i386/reload.ml | 18 +++++++++++++++++- 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/asmcomp/amd64/reload.ml b/asmcomp/amd64/reload.ml index 1f4cadc391b5..98c3914633f2 100644 --- a/asmcomp/amd64/reload.ml +++ b/asmcomp/amd64/reload.ml @@ -33,7 +33,7 @@ open Mach Iload R R R Istore R R Iintop(Icomp) R R S - or S S R + or R S R Iintop(Imul|Idiv|Imod) R R S Iintop(Imulh) R R S Iintop(shift) S S R @@ -41,6 +41,7 @@ open Mach or S S R Iintop_imm(Iadd, n)/lea R R Iintop_imm(Imul, n) R R + Iintop_imm(Icomp, n) R S Iintop_imm(others) S S Inegf...Idivf R R S Ifloatofint R S @@ -66,7 +67,18 @@ inherit Reloadgen.reload_generic as super method! reload_operation op arg res = match op with - | Iintop(Iadd|Isub|Iand|Ior|Ixor|Icomp _|Icheckbound) -> + | Iintop(Iadd|Isub|Iand|Ior|Ixor|Icheckbound) -> + (* One of the two arguments can reside in the stack, but not both *) + if stackp arg.(0) && stackp arg.(1) + then ([|arg.(0); self#makereg arg.(1)|], res) + else (arg, res) + | Iintop(Icomp _) -> + (* The result must be a register *) + let res = + if stackp res.(0) + then [|self#makereg res.(0)|] + else res + in (* One of the two arguments can reside in the stack, but not both *) if stackp arg.(0) && stackp arg.(1) then ([|arg.(0); self#makereg arg.(1)|], res) @@ -80,6 +92,11 @@ method! reload_operation op arg res = if stackp arg.(0) then (let r = self#makereg arg.(0) in ([|r|], [|r|])) else (arg, res) + | Iintop_imm(Icomp _, _) -> + (* The result must be in a register *) + if stackp res.(0) + then (arg, [|self#makereg res.(0)|]) + else (arg, res) | Iintop(Imulh | Idiv | Imod | Ilsl | Ilsr | Iasr) | Iintop_imm(_, _) -> (* The argument(s) and results can be either in register or on stack *) diff --git a/asmcomp/i386/reload.ml b/asmcomp/i386/reload.ml index 09497e050751..83dfed28e9a3 100644 --- a/asmcomp/i386/reload.ml +++ b/asmcomp/i386/reload.ml @@ -40,7 +40,18 @@ method! makereg r = method! reload_operation op arg res = match op with - Iintop(Iadd|Isub|Iand|Ior|Ixor|Icomp _|Icheckbound) -> + Iintop(Iadd|Isub|Iand|Ior|Ixor|Icheckbound) -> + (* One of the two arguments can reside in the stack *) + if stackp arg.(0) && stackp arg.(1) + then ([|arg.(0); self#makereg arg.(1)|], res) + else (arg, res) + | Iintop(Icomp _) -> + (* The result must be a register *) + let res = + if stackp res.(0) + then [|self#makereg res.(0)|] + else res + in (* One of the two arguments can reside in the stack *) if stackp arg.(0) && stackp arg.(1) then ([|arg.(0); self#makereg arg.(1)|], res) @@ -60,6 +71,11 @@ method! reload_operation op arg res = if stackp arg.(0) then let r = self#makereg arg.(0) in ([|r|], [|r|]) else (arg, res) + | Iintop_imm(Icomp _, _) -> + (* The result must be in a register *) + if stackp res.(0) + then (arg, [|self#makereg res.(0)|]) + else (arg, res) | Iintop(Imulh | Ilsl | Ilsr | Iasr) | Iintop_imm(_, _) | Ifloatofint | Iintoffloat | Ispecific(Ipush) -> (* The argument(s) can be either in register or on stack *) From 81dd82adc0315b96d19a691eb2cd0f1dd9f40289 Mon Sep 17 00:00:00 2001 From: Vincent Laviron Date: Mon, 12 Dec 2022 12:15:40 +0100 Subject: [PATCH 2/3] Use self#makeregs directly --- asmcomp/amd64/reload.ml | 14 ++++---------- asmcomp/i386/reload.ml | 14 ++++---------- asmcomp/reloadgen.ml | 2 +- asmcomp/reloadgen.mli | 1 + 4 files changed, 10 insertions(+), 21 deletions(-) diff --git a/asmcomp/amd64/reload.ml b/asmcomp/amd64/reload.ml index 98c3914633f2..9e3cadbe4402 100644 --- a/asmcomp/amd64/reload.ml +++ b/asmcomp/amd64/reload.ml @@ -73,12 +73,8 @@ method! reload_operation op arg res = then ([|arg.(0); self#makereg arg.(1)|], res) else (arg, res) | Iintop(Icomp _) -> - (* The result must be a register *) - let res = - if stackp res.(0) - then [|self#makereg res.(0)|] - else res - in + (* The result must be a register (PR#11803) *) + let res = self#makeregs res in (* One of the two arguments can reside in the stack, but not both *) if stackp arg.(0) && stackp arg.(1) then ([|arg.(0); self#makereg arg.(1)|], res) @@ -93,10 +89,8 @@ method! reload_operation op arg res = then (let r = self#makereg arg.(0) in ([|r|], [|r|])) else (arg, res) | Iintop_imm(Icomp _, _) -> - (* The result must be in a register *) - if stackp res.(0) - then (arg, [|self#makereg res.(0)|]) - else (arg, res) + (* The result must be in a register (PR#11803) *) + (arg, self#makeregs res) | Iintop(Imulh | Idiv | Imod | Ilsl | Ilsr | Iasr) | Iintop_imm(_, _) -> (* The argument(s) and results can be either in register or on stack *) diff --git a/asmcomp/i386/reload.ml b/asmcomp/i386/reload.ml index 83dfed28e9a3..6a20e887bacd 100644 --- a/asmcomp/i386/reload.ml +++ b/asmcomp/i386/reload.ml @@ -46,12 +46,8 @@ method! reload_operation op arg res = then ([|arg.(0); self#makereg arg.(1)|], res) else (arg, res) | Iintop(Icomp _) -> - (* The result must be a register *) - let res = - if stackp res.(0) - then [|self#makereg res.(0)|] - else res - in + (* The result must be a register (PR#11803) *) + let res = self#makeregs res in (* One of the two arguments can reside in the stack *) if stackp arg.(0) && stackp arg.(1) then ([|arg.(0); self#makereg arg.(1)|], res) @@ -72,10 +68,8 @@ method! reload_operation op arg res = then let r = self#makereg arg.(0) in ([|r|], [|r|]) else (arg, res) | Iintop_imm(Icomp _, _) -> - (* The result must be in a register *) - if stackp res.(0) - then (arg, [|self#makereg res.(0)|]) - else (arg, res) + (* The result must be in a register (PR#11803) *) + (arg, self#makeregs res) | Iintop(Imulh | Ilsl | Ilsr | Iasr) | Iintop_imm(_, _) | Ifloatofint | Iintoffloat | Ispecific(Ipush) -> (* The argument(s) can be either in register or on stack *) diff --git a/asmcomp/reloadgen.ml b/asmcomp/reloadgen.ml index d9c707164b2a..c000dd4aeaad 100644 --- a/asmcomp/reloadgen.ml +++ b/asmcomp/reloadgen.ml @@ -46,7 +46,7 @@ method makereg r = newr.spill_cost <- 100000; newr -method private makeregs rv = +method makeregs rv = let n = Array.length rv in let newv = Array.make n Reg.dummy in for i = 0 to n-1 do newv.(i) <- self#makereg rv.(i) done; diff --git a/asmcomp/reloadgen.mli b/asmcomp/reloadgen.mli index 638082f0a718..0cf264c43416 100644 --- a/asmcomp/reloadgen.mli +++ b/asmcomp/reloadgen.mli @@ -20,6 +20,7 @@ class reload_generic : object (* Can be overridden to reflect instructions that can operate directly on stack locations *) method makereg : Reg.t -> Reg.t + method makeregs : Reg.t array -> Reg.t array (* Can be overridden to avoid creating new registers of some class (i.e. if all "registers" of that class are actually on stack) *) method fundecl : Mach.fundecl -> int array -> Mach.fundecl * bool From 11bed793e4741be230e7ef673c07f7509b190c85 Mon Sep 17 00:00:00 2001 From: Xavier Leroy Date: Wed, 14 Dec 2022 16:17:17 +0100 Subject: [PATCH 3/3] Add Changes entry for 11803 --- Changes | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Changes b/Changes index 3adce809b57a..7f9d3ffd934d 100644 --- a/Changes +++ b/Changes @@ -3,6 +3,12 @@ OCaml 4.14 maintenance branch ### Bug fixes: +- #11803, #11808: on x86, the destination of an integer comparison must be + a register, it cannot be a stack slot. + (Vincent Laviron, review by Xavier Leroy, report by + Emilio Jesús Gallego Arias) + + OCaml 4.14.1 -----------------------------