Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[RISCV] Explicitly select second operand of branch condition to X0.

Authored by craig.topper on Jul 29 2022, 4:49 PM.



At least based on the lit tests, the coalescer sometimes fail to
propagate the copy from X0 into the branch instruction. This patch
does it manually during isel. The majority of the changes are from
the select patterns.

Some of the changes are just register allocation changes. Only
the Select change affects the whether a b*z instruction is generated
in the tests. I changed the branch pattern for consistency.

Diff Detail

Event Timeline

craig.topper created this revision.Jul 29 2022, 4:49 PM
Herald added a project: Restricted Project. · View Herald Transcript
craig.topper requested review of this revision.Jul 29 2022, 4:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 29 2022, 4:49 PM
asb accepted this revision.Jul 31 2022, 10:12 PM

I'm obviously a big fan of the codegen improvements resulting from this patch...but it always feels unpleasant to add a target-specific workaround for what seems to be a bug/limitation elsewhere. I think it does make sense to land this workaround as it's not clear how feasible improving the coalescer would be, but perhaps you could file an issue to track this problem?

This revision is now accepted and ready to land.Jul 31 2022, 10:12 PM
reames added a comment.Aug 1 2022, 8:20 AM

LGTM as well. I think this is worth taking, but as noted, I think we may have more oddities to dig into here.


I think there's still a problem with this code. This initialization of a1 appears to never be clobbered, and yet we have multiple register copies below instead of zero initialization. It's less immediate problematic than the branch interaction, but something odd is still going on here.

This revision was landed with ongoing or failed builds.Aug 1 2022, 11:17 AM
This revision was automatically updated to reflect the committed changes.
asb added a comment.Aug 2 2022, 1:56 AM

I've filed to at least track this, as it would be great if target-independent improvements meant we could remove these patterns again.