[PATCH] D76992: [VPlan] Add & use VPValue operands for VPWidenRecipe (NFC).

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4291
+      Value *A = State.get(User.getOperand(0), Part);
+      Value *B = State.get(User.getOperand(1), Part);
       Value *C = nullptr;
----------------
fhahn wrote:
> bkramer wrote:
> > mehdi_amini wrote:
> > > mehdi_amini wrote:
> > > > FYI XLA generated IR triggers a crash here.
> > > > 
> > > > I have the IR right before the pass (I can share it, this is the public XLA test-suite) but I can't reproduce with `opt -loop-vectorize` (I suspect some TTI/TLI settings are affecting this).
> > > > 
> > > > I don't really understand this code so I couldn't debug it right now but in gdb:
> > > > ```
> > > > (gdb) call User.getUnderlyingValue() 
> > > > $13 = (llvm::Value *) 0x0
> > > > (gdb) call User.getOperand(0)->getUnderlyingValue()->dump()
> > > >   %reduce-window.10.clone.invar_address.dim.3.016 = phi i64 [ %bc.resume.val, %scalar.ph ], [ %invar.inc3, %reduce-window.10.clone.inner.loop_exit.window.0 ]
> > > > (gdb) call User.getOperand(1)->getUnderlyingValue()->dump()
> > > > i64 13
> > > > (gdb) p Part
> > > > $14 = 0
> > > > ```
> > > > 
> > > > I'll likely revert while we're trying to extract a minimum reproducer. Let me know if you have any advice for this?
> > > > 
> > > While I have a gdb session opened:
> > > 
> > > ```
> > > #0  llvm::Value::getType (this=0x0) at llvm/include/llvm/IR/Value.h:244
> > > #1  0x0000555596796922 in llvm::InnerLoopVectorizer::getOrCreateVectorValue (this=0x7fffee60fae0, V=0x0, Part=0) at llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2020
> > > #2  0x00005555967b1a87 in llvm::LoopVectorizationPlanner::VPCallbackILV::getOrCreateVectorValues (this=0x7fffee60f8a8, V=0x0, Part=0)
> > >     at llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7376
> > > #3  0x00005555967bd8fd in llvm::VPTransformState::get (this=0x7fffee60f770, Def=0x37e73a4572d0, Part=0) at llvm/lib/Transforms/Vectorize/VPlan.h:270
> > > #4  0x00005555967a211e in llvm::InnerLoopVectorizer::widenInstruction (this=0x7fffee60fae0, I=..., User=..., State=...) at llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4292
> > > #5  0x00005555967b1d65 in llvm::VPWidenRecipe::execute (this=0x37e73a450b60, State=...) at llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7412
> > > #6  0x00005555968488a0 in llvm::VPBasicBlock::execute (this=0x37e73aa4c360, State=0x7fffee60f770) at llvm/lib/Transforms/Vectorize/VPlan.cpp:240
> > > #7  0x0000555596849c71 in llvm::VPlan::execute (this=0x37e73cbcc260, State=0x7fffee60f770) at llvm/lib/Transforms/Vectorize/VPlan.cpp:476
> > > #8  0x00005555967ae456 in llvm::LoopVectorizationPlanner::executePlan (this=0x7fffee610080, ILV=..., DT=0x37e73b2db0c0) at llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6612
> > > #9  0x00005555967b4181 in llvm::LoopVectorizePass::processLoop (this=0x37e73c142860, L=0x37e73cc9d2a0) at llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7919
> > > ```
> > https://reviews.llvm.org/P8215 crashes when run through `opt -O3`. Sadly bugpoint doesn't seem to like it so it's a huge test case.
> Thanks, I'll take a look.
This patch exposed a bug in how backedge-taken-count is recorded; uploaded a fix in D78847.
Would be good to confirm if the two patches combined resolve the original issue(s).


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:270
       return Data.PerPartOutput[Def][Part];
     // Def is managed by ILV: bring the Values from ValueMap.
     return Callback.getOrCreateVectorValues(VPValue2Value[Def], Part);
----------------
Would be good to add an assert of VPValue2Value.count(Def), instead of creating a entry with null Value leading to a crash, as reported.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:277
   /// this method currently just delegates the call to ILV.
   Value *get(VPValue *Def, const VPIteration &Instance) {
     return Callback.getOrCreateScalarValue(VPValue2Value[Def], Instance);
----------------
ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76992