This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Lower scalable vector masked loads to intrinsics to match fixed vectors and reduce isel patterns.
ClosedPublic

Authored by craig.topper on Mar 17 2021, 7:23 PM.

Diff Detail

Event Timeline

craig.topper created this revision.Mar 17 2021, 7:23 PM
craig.topper requested review of this revision.Mar 17 2021, 7:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2021, 7:23 PM
Herald added a subscriber: MaskRay. · View Herald Transcript
craig.topper added inline comments.Mar 17 2021, 7:24 PM
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.

craig.topper added inline comments.Mar 17 2021, 7:48 PM
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.

frasercrmck added inline comments.Mar 18 2021, 3:33 AM
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.

craig.topper added inline comments.Mar 18 2021, 4:51 PM
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.

This revision is now accepted and ready to land.Mar 19 2021, 2:31 AM