Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
3235 | There was a bug here. We were returning the old chain. Need to figure out why that didn't break anything. The fixed vector tests have a store following the load so that chain should have been alive. |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
3235 | Oh we returned the input chain so we skipped having the new load connected to its dependent operations. That's why it worked. I was thinking it was the output chain from the old load. How concerned are we with having a test that would have broken? I'll have to come up with something where we accidentally reorder incorrectly because of it. You need a store or something after the load that isn't dependent on the data returned by load and show that the load sinks below it. |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
3235 | Ach, sorry I missed that during review. Hmm, I think this would be difficult to reproduce in a test. I wonder if you could write a test that checks the ScheduleDAG SUnit dependencies somehow. I think we could live without it, though. |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
3235 | The only way I know to check the dependencies is to grep the debug output. I went ahead and committed the fix separately so it wasn't hidden in this review. |
There was a bug here. We were returning the old chain. Need to figure out why that didn't break anything. The fixed vector tests have a store following the load so that chain should have been alive.