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

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:3222
+
+  return DAG.getMergeValues({Result, Chain}, DL);
 }
----------------
craig.topper wrote:
> craig.topper wrote:
> > 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.
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98840/new/

https://reviews.llvm.org/D98840