On 10/25/23 01:12, Robin Dapp wrote:
Hi Vineet,
I was thinking of two things while skimming the code:
- Couldn't we do this in the expanders directly? Or is the
subreg-promoted info gone until we reach that?
Well, it doesn't seem like there's a lot of difference between doing it
in the generic expander bits vs target expander bits -- the former just
calls into the latter for the most part. Thus if the subreg-promoted
state is available in the target expander, I'd expect it to be available
in the generic expander.
It may be the case that we have more places to fix because we have
different expander paths (think conditional branches, conditional moves,
sCC and probably others). By catching it in riscv_expand_comparands he
caught a nice little choke point. I think what Vineet has done will
also work for RTL if-conversion.
- Should some common-code part be more suited to handle that?
We already elide redundant sign-zero extensions for other
reasons. Maybe we could add subreg promoted handling there?
Unsure. I wouldn't be surprised if we were able to find similar code in
simplify-rtx or something like that. It's probably worth a quick looksie.
I also wonder if Vineet's work would subsume this local change. I've
been meaning to find testcases for this and determine if we should drop
it or clean it up and submit it upstream:
+(define_insn "*branch<mode>_equals_zero"
+ [(set (pc)
+ (if_then_else
+ (match_operator 1 "equality_operator"
+ [(match_operand:ANYI 2 "register_operand" "r")
+ (const_int 0)])
+ (label_ref (match_operand 0 "" ""))
+ (pc)))]
+ "!partial_subreg_p (operands[2])"
+ "b%C1\t%2,zero,%0"
+ [(set_attr "type" "branch")
+ (set_attr "mode" "none")])
My sense is it's just papering over a missed simplification elsewhere.
Jeff
