[PATCH] D138238: [SROA] For non-speculatable `load`s of `select`s -- split block, insert then/else blocks, form two-entry PHI node

[Michael Kitzan via Phabricator via llvm-commits]

mkitzan added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:1409
+  LoadInst *TL =
+      IRB.CreateAlignedLoad(LI.getType(), TV, LI.getAlign(),
+                            LI.getName() + ".sroa.speculate.load.true");
----------------
lebedev.ri wrote:
> nikic wrote:
> > lebedev.ri wrote:
> > > mkitzan wrote:
> > > > lebedev.ri wrote:
> > > > > lebedev.ri wrote:
> > > > > > mkitzan wrote:
> > > > > > > Our out-of-tree backend is asserting in this function call (particularly at `assert(cast<PointerType>(Ptr->getType())->isOpaqueOrPointeeTypeMatches(Ty))` which occurs while creating the load). I suspect this is due to the lost bitcasts, since when I add them back there's no assert.
> > > > > > > 
> > > > > > > The code I have adding the bitcasts back looks like the following:
> > > > > > > ```
> > > > > > >   IRB.SetInsertPoint(&LI);
> > > > > > > 
> > > > > > >   // OUT OF TREE
> > > > > > >   if (SI.hasOneUse()) {
> > > > > > >     if (BitCastInst *BC = dyn_cast<BitCastInst>(SI.user_back())) {
> > > > > > >       // assert(BC->user_back() == &LI); <- unclear if this is guaranteed
> > > > > > >       TV = IRB.CreateBitCast(TV, BC->getType(), TV->getName() + ".sroa.cast");
> > > > > > >       FV = IRB.CreateBitCast(FV, BC->getType(), FV->getName() + ".sroa.cast");
> > > > > > >     }
> > > > > > >   }
> > > > > > >   // END OUT OF TREE
> > > > > > > 
> > > > > > >   LoadInst *TL = ...
> > > > > > > ```
> > > > > > > 
> > > > > > > To be honest, I have not entirely analyzed this patch, so I'm not confident that the way I have added the bitcasts back is legal (especially now that there's no while loop on SI's uses...). Could you advise if this is legal/correct (to unblock us)? I can try to get an anonymized test case for this, so a full fix can be made.
> > > > > > We are literally days away from stopping accepting such fixes.
> > > > > > That being said, let me fix this one...
> > > > > 9f27f4536e19e93349b0662338408efe6d1cb2fd, but it is probably one of last ones of it's kind.
> > > > Thanks for the quick turn around 
> > > Sorry, that was too quick and it got reverted.
> > > 
> > > @nikic please can you estimate when we switch to not dealing with typed pointer bugs?
> > Here's my proposal for a hard timeline: https://reviews.llvm.org/D140487 (tl;dr supported until release/16.x branch creation on Jan 24th).
> 2cb393590ea537b06fa66e6d847a1159c227a313, looks like it stuck this time around.
Haha no worries. Glad it's finally landed 👍


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138238