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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98840