- User Since
- Sep 26 2016, 7:58 AM (160 w, 13 h)
Thanks for the patience with all my remarks, turning this around completely compared to the original patch.
Fri, Oct 18
Thanks! I like this solution.
Wed, Oct 16
- Fixup mistake regarding FDiv (it was not supposed to be included in the new restriction).
- Added test cases for sdiv/urem/udiv (showing that the new restriction works).
- Added test cases for fdiv/frem (showing that we can eliminate the shufflevector).
Tue, Oct 15
Fri, Oct 11
Thu, Oct 10
I'm still confused. IMO moving or not moving dbg instructions is a different question (not being discussed here). Still no explanation why result differs depending on if we move dbg instructuons or not. And this patch now blindly moves some dbg instructions, regardless if they are related to the allocas or not. Consider an instruction sequence like this:
alloca bitcast dbg.declare bitcast alloca bitcast dbg.value
The scanning for allocas will find the two alloca instructions and call splice twice, only moving the alloca instructions, right?
Wed, Oct 9
Tue, Oct 8
I do not understand how this helps. The code is written in a way that it skips any instruction, but moves contigous blocks of allocas in one splice (not sure exactly why, is that really faster?). Maybe the difference is that the check for AI->useEmpty() only is done for the first alloca in a sequence of alloca instructions? Or can't we just remove the loop at line 1847 (only moving one alloca at a time).
Thu, Sep 26
I don't think we need to talk about the "edge cases" at all any longer (after reading the comment from Roman once more, and analysing the code a little bit).
Wed, Sep 25
Sep 20 2019
Sep 18 2019
Sorry, I haven't had time to look at this.
Sep 16 2019
Not pursuing this. Should have been abandoned a long time ago.
Sep 13 2019
Sep 11 2019
Sep 7 2019
Sep 6 2019
Now using std::numeric_limits and C++ style casts (thanks JDevlieghere!).
Sep 5 2019
(totally understand if tihs hasn't been on top of prio list for reviewers, and that it might be a lot to digest)
Sep 4 2019
LGTM (apart from the comment about maybe saying something about the iteration order).
Sep 3 2019
Thanks for all the feedback. Landed as rL370721.
Sep 2 2019
Updated description for AllowedExit.
Fixes to get umul.fix.sat on par with smul.fix.sat. Basically we now handle umul.fix.sat wherever we handled smul.fix.sat (and UMULFIXSAT wherever we handle SMULFIXSAT), except for ConstantFolding.
Sep 1 2019
Some fixes are planned:
- Fix problem with scale==0 spotted in TargetLowering::expandFixedPointMul
- Add fixes that has been added for smul.fix.sat since this patch originally was put up for review (isTriviallyVectorizable, VectorLegalizer::Expand, DAGTypeLegalizer::WidenVectorResult, DAGCompielr::visitMULFIX).
Aug 31 2019
Commandeered (from leonardchan).
Now being based on monorepo.
Fixed bugs in LangRef.
Fixed bugs in ExpandIntRes_MULFIX (basically reimplemented saturation for UMULFIXSAT after breaking it out into a separate if-clause (no longer implementing shift by scale, signed saturation and unsigned saturation in the same piece of code).
Aug 30 2019
Aug 29 2019
(just sharing my thoughts after seeing this patch)
Aug 27 2019
Update: Renamed updatePHIs as replacePhiUsesOfBlockWith. Name choosen to "match" the already existing ReplaceUsesOfBlockWith which basically does the same thing but for all instructions in the block.
Aug 26 2019
Not sure if we need it for this patch, but I remember that last time I did something with computeNumSignBits in SelectionDAG I was informed that it is possible to add unittests in llvm/unittests/CodeGen/AArch64SelectionDAGTest.cpp. In case you for example want to make more detailed tests.
FYI: I made a fixup here https://reviews.llvm.org/rL369891