-
Notifications
You must be signed in to change notification settings - Fork 200
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.
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
poor codegen for rtio_output #667
Comments
Here's the LLVM patch that fixes this: diff --git a/lib/Target/OR1K/OR1KISelDAGToDAG.cpp b/lib/Target/OR1K/OR1KISelDAGToDAG.cpp
index 0f4820db2ad..10a39e86d6a 100644
--- a/lib/Target/OR1K/OR1KISelDAGToDAG.cpp
+++ b/lib/Target/OR1K/OR1KISelDAGToDAG.cpp
@@ -143,6 +143,18 @@ SelectAddr(SDValue Addr, SDValue &Base, SDValue &Offset) {
}
}
+ // Fold the ORI in MOVHI->ORI->LW/SW chains into LW/SW, if possible.
+ if (ConstantSDNode *CN = dyn_cast<ConstantSDNode>(Addr.getNode())) {
+ uint64_t AddrImm = CN->getZExtValue();
+ // The LW/SW offset is sign-extended, and we want to avoid subtraction.
+ if(AddrImm & 0x8000 == 0) {
+ SDValue AddrHigh = CurDAG->getTargetConstant(AddrImm >> 16, dl, MVT::i32);
+ Base = SDValue(CurDAG->getMachineNode(OR1K::MOVHI, dl, MVT::i32, AddrHigh), 0);
+ Offset = CurDAG->getTargetConstant(AddrImm & 0x7FFF, dl, MVT::i32);
+ return true;
+ }
+ }
+
Base = Addr;
Offset = CurDAG->getTargetConstant(0, dl, MVT::i32);
return true; Unfortunately it (while correctly changing codegen) pessimizes test_pulse_rate_dds for unknown reasons, and has a negligible effect on test_pulse_rate. Therefore it doesn't seem worth integrating. |
Doesn't this indicate that there is a bigger bug somewhere else? |
@jordens Maybe, depending on how you define "bug". As @sbourdeauducq has mentioned elsewhere the root cause of this could be cache aliasing. Unfortunately MiSoC CPU &c cores do not currently provide any insight into their operation--there are no performance counters or anything. The most I could do is a sampling profiler, based on libunwind, but I don't think that will help this issue. Arguably we should look into it. However, I'm not sure how this fits into the overall roadmap, and this is definitely a nontrivial change--we're looking, at the very least, at a fork of mor1kx and accompanying infrastructure changes, which is annoying enough already. |
Per discussion with @jordens the fix should be merged and test condition relaxed (until the other bug is fixed). |
Before:
After:
|
RISC-V code seems fine.
|
instead of
The text was updated successfully, but these errors were encountered: