bnjbvr opened PR #2071 from x64-xor
to main
:
This shows how we can factor out helpers for knowing whether an instruction redefines its input instead of using it.
bnjbvr requested cfallin and julian-seward1 for a review on PR #2071.
bnjbvr requested cfallin and julian-seward1 for a review on PR #2071.
cfallin submitted PR Review.
cfallin submitted PR Review.
cfallin created PR Review Comment:
This reads a little awkwardly (three cheers for expression-
if
s, but...). Perhaps add a helper toRegMemImm
such as.to_reg() -> Option<Reg>
, then you can doop == ... && src.to_reg() == Some(dst.to_reg()
?
cfallin created PR Review Comment:
We should have a comment here describing what's going on, perhaps similar to my suggestion in @abrown's patch here.
abrown submitted PR Review.
abrown created PR Review Comment:
Another thought: why not make these methods of
Inst
? Then we can use this logic withinst.is_def()
and avoid searching around foralu_rmi_r_is_def
?
abrown submitted PR Review.
abrown created PR Review Comment:
What do you think of
map_as_def
? I don't feel too strongly about that but I think it would pretty helpful to explain here what is going on (e.g. we have to pretend the register is writable becausemap_def
only accepts writable registers).
julian-seward1 submitted PR Review.
julian-seward1 created PR Review Comment:
I like the expression-
if
, but maybe it could be made clearer by changing the formal parameters thusly:fn alu_rmi_r_is_def(rmi: &RegMemImm, reg: Writable<Reg>, op: AluRmiROpcode)
Other comments:
- I would find the name
alu_rmi_r_is_const
more natural; could that be considered?- also
op == AluRmiROpcode::Sub
behaves similarly, and really we should handle that too, even though we don't expect anybody to actually use it
julian-seward1 submitted PR Review.
julian-seward1 created PR Review Comment:
Just as a sanity check .. do we need special-case handling for
map_regs
? IIUC, if there is no special-case handling then the mapping for thereg
component of the insn will assert, complaining that thedef
anduse
maps are inconsistent .. yes? Can you confirm?
julian-seward1 submitted PR Review.
julian-seward1 created PR Review Comment:
Or maybe
alu_rmi_r_produces_const
..
julian-seward1 submitted PR Review.
julian-seward1 created PR Review Comment:
Also
cmpeqxx
or whatever the recommended generate-all-1s idiom is. The Intel opt guide suggests this is recommended for Sandy Bridge onwards.
julian-seward1 submitted PR Review.
abrown submitted PR Review.
abrown created PR Review Comment:
I add that in #2060.
bnjbvr submitted PR Review.
bnjbvr created PR Review Comment:
I like it!
bnjbvr submitted PR Review.
bnjbvr created PR Review Comment:
I didn't run into any issues with this, and i could confirm it was probably used. This patch does adjust both reg usage collection and reg usage mapping, so nothing should be missing as far as i can tell.
bnjbvr submitted PR Review.
bnjbvr created PR Review Comment:
Thanks for the comments! To sum up:
- created it as a method of Inst
- called
produces_const()
- and added helper
to_reg
to both RegMem and RegMemImm
bnjbvr updated PR #2071 from x64-xor
to main
:
This shows how we can factor out helpers for knowing whether an instruction redefines its input instead of using it.
abrown merged PR #2071.
Last updated: Dec 23 2024 at 13:07 UTC