Skip to content
  1. Jan 03, 2020
  2. Jan 02, 2020
    • Sanjay Patel's avatar
    • Sanjay Patel's avatar
      [InstCombine] remove uses before deleting instructions (PR43723) · 88fc5fde
      Sanjay Patel authored
      This is a less ambitious alternative to previous attempts to fix
      this bug with:
      rG56b2aee1875a
      rGef02831f0a4e
      rG56b2aee1875a
      ...because those all failed bot testing with use-after-free or
      other problems.
      
      The original crashing/assert problem is still showing up on
      various fuzzers, so I've added a new minimal test based on
      another one of those failures.
      
      Instead of trying to manage and coordinate the logic in
      isAllocSiteRemovable() with the deletion loops, just loosen
      the existing code that handles casts and GEP by replacing
      with undef to allow other opcodes. That means that no
      instructions with uses should assert on deletion, and there
      are hopefully no non-obvious sanitizer bugs induced.
      88fc5fde
  3. Jan 01, 2020
  4. Dec 31, 2019
  5. Dec 30, 2019
  6. Dec 25, 2019
  7. Dec 23, 2019
  8. Dec 22, 2019
  9. Dec 21, 2019
  10. Dec 20, 2019
  11. Dec 19, 2019
    • Roman Lebedev's avatar
      [ValueTracking] isKnownNonZero() should take non-null-ness assumptions into consideration (PR43267) · 047186cc
      Roman Lebedev authored
      Summary:
      It is pretty common to assume that something is not zero.
      Even optimizer itself sometimes emits such assumptions
      (e.g. `addAssumeNonNull()` in `PromoteMemoryToRegister.cpp`).
      
      But we currently don't deal with such assumptions :)
      The only way `isKnownNonZero()` handles assumptions is
      by calling `computeKnownBits()` which calls `computeKnownBitsFromAssume()`.
      But `x != 0` does not tell us anything about set bits,
      it only says that there are *some* set bits.
      So naturally, `KnownBits` does not get populated,
      and we fail to make use of this assumption.
      
      I propose to deal with this special case by special-casing it
      via adding a `isKnownNonZeroFromAssume()` that returns boolean
      when there is an applicable assumption.
      
      While there, we also deal with other predicates,
      mainly if the comparison is with constant.
      
      Fixes [[ https://bugs.llvm.org/show_bug.cgi?id=43267 | PR43267 ]].
      
      Differential Revision: https://reviews.llvm.org/D71660
      047186cc
    • Roman Lebedev's avatar
      [ValueTracking] isValidAssumeForContext(): CxtI itself also must transfer execution to successor · 92083a29
      Roman Lebedev authored
      This is a pretty rare case, when CxtI and assume are
      in the same basic block, with assume being located later.
      
      We were already checking that assumption was guaranteed to be
      executed, but we omitted CxtI itself from consideration,
      and as the test (miscompile) shows, that is incorrect.
      
      As noted in D71660 review by @nikic.
      92083a29
    • Roman Lebedev's avatar
      [NFC][InstCombine] Add a test for assume-induced miscompile · ffcae008
      Roman Lebedev authored
      @escape() may throw here, we don't know that assumption, which is located
      afterwards in the same block, is executed, therefore %load arg of
      call to @escape() can not be marked as non-null.
      
      As noted in D71660 review by @nikic.
      ffcae008
    • Sanjay Patel's avatar
      [InstCombine] add/adjust tests for pow->sqrt; NFC · 5889e782
      Sanjay Patel authored
      There's at least 1 bug here as discussed in PR44330.
      5889e782
    • David Green's avatar
      [InstCombine] Canonicalize select immediates · a59cc5e1
      David Green authored
      In certain situations after inlining and simplification we end up with
      code that is _almost_ a min/max pattern, but contains constants that
      have been demand-bit optimised to the wrong values, ending up with code
      like:
        %1 = icmp slt i32 %shr, -128
        %2 = select i1 %1, i32 128, i32 %shr
        %.inv = icmp sgt i32 %shr, 127
        %spec.select.i = select i1 %.inv, i32 127, i32 %2
        %conv7 = trunc i32 %spec.select.i to i8
      This should be turned into a min/max pattern, but the -128 in the first
      select was instead transformed into 128, as only the bottom byte was
      ever demanded.
      
      To fix this, I've put in further canonicalisation for the immediates of
      selects, preferring to use the same value as the icmp if available.
      
      Differential Revision: https://reviews.llvm.org/D71516
      a59cc5e1
    • David Green's avatar
      d3815332
  12. Dec 18, 2019
    • Piotr Sobczak's avatar
      Revert "[InstCombine][AMDGPU] Trim more components of *buffer_load" · 40b5a0f7
      Piotr Sobczak authored
      Revert D70315, as it breaks gfx8 for some reason.
      
      This reverts commit 65f94b33.
      40b5a0f7
    • Jakub Kuderski's avatar
      [InstCombine] Insert instructions before adding them to worklist · 3d29c41a
      Jakub Kuderski authored
      Summary:
      This patch adds instructions to the InstCombine worklist after they are properly inserted. This way we don't get `<badref>`s printed when logging added instructions.
      It also adds a check in `Worklist::Add` that ensures that all added instructions have parents.
      
      Simple test case that illustrates the difference when run with `--debug-only=instcombine`:
      
      ```
      define i32 @test35(i32 %a, i32 %b) {
        %1 = or i32 %a, 1135
        %2 = or i32 %1, %b
        ret i32 %2
      }
      ```
      
      Before this patch:
      ```
      INSTCOMBINE ITERATION #1 on test35
      IC: ADDING: 3 instrs to worklist
      IC: Visiting:   %1 = or i32 %a, 1135
      IC: Visiting:   %2 = or i32 %1, %b
      IC: ADD:   %2 = or i32 %a, %b
      IC: Old =   %3 = or i32 %1, %b
          New =   <badref> = or i32 %2, 1135
      IC: ADD:   <badref> = or i32 %2, 1135
      ...
      ```
      
      With this patch:
      ```
      INSTCOMBINE ITERATION #1 on test35
      IC: ADDING: 3 instrs to worklist
      IC: Visiting:   %1 = or i32 %a, 1135
      IC: Visiting:   %2 = or i32 %1, %b
      IC: ADD:   %2 = or i32 %a, %b
      IC: Old =   %3 = or i32 %1, %b
          New =   <badref> = or i32 %2, 1135
      IC: ADD:   %3 = or i32 %2, 1135
      ...
      ```
      
      Reviewers: fhahn, davide, spatel, foad, grosser, nikic
      
      Reviewed By: nikic
      
      Subscribers: nikic, lebedev.ri, hiraditya, llvm-commits
      
      Tags: #llvm
      
      Differential Revision: https://reviews.llvm.org/D71093
      3d29c41a
    • Jakub Kuderski's avatar
      [InstCombine] Allow to limit the max number of iterations · 406b6019
      Jakub Kuderski authored
      Summary:
      This patch teaches InstCombine to accept a new parameter: maximum number of iterations over functions.
      
      InstCombine tries to simplify instructions by iterating over the whole function until the function stops changing. As a consequence, the last iteration before reaching a fixpoint visits all instructions in the worklist and never performs any rewrites.
      
      Bounding the number of iterations can have 2 benefits:
      * In case the users of the pass can make a good guess about the number of required iterations, we can save the time normally spent on the last iteration that doesn't change anything.
      * When the wants to use InstCombine as a cleanup pass, it may be enough to run just a few iterations and stop even before reaching a fixpoint. This can be also useful for implementing a lightweight pass pipeline (think `-O1`).
      
      This patch does not change the behavior of opt or Clang -- limiting the number of iterations is entirely opt-in.
      
      Reviewers: fhahn, davide, spatel, foad, nlopes, grosser, lebedev.ri, nikic, xbolva00
      
      Reviewed By: spatel
      
      Subscribers: craig.topper, hiraditya, llvm-commits
      
      Tags: #llvm
      
      Differential Revision: https://reviews.llvm.org/D71145
      406b6019
Loading