[PATCH] D150388: [CodeGen]Allow targets to use target specific COPY instructions for live range splitting

[Alexander Kornienko via Phabricator via llvm-commits]

alexfh added a comment.

In D150388#4516897 <https://reviews.llvm.org/D150388#4516897>, @arsenm wrote:

> I see the diff disappear if I make isCopyInstr skip isCopyInstrImpl. I'm not seeing anything clearly wrong with the codegen decisions here. The apparent block body removal in %bb.4, and the implicit_def; kill in the entry look suspicious, but I'm not seeing how it's wrong.

I've tried replacing `isCopyInstrImpl()` call in `isCopyInstr()` with a `return std::nullopt;` and the issue disappeared. I don't know how my experiment is different to what @asmok-g did, but this definitely helps. I tried this in two configurations:

- llvm from f3d0613d852a90563a1e8704930a6e79368f106a <https://reviews.llvm.org/rGf3d0613d852a90563a1e8704930a6e79368f106a> + this commit (b7836d856206ec39509d42529f958c920368166b <https://reviews.llvm.org/rGb7836d856206ec39509d42529f958c920368166b>) - tests fail with `UndefinedBehaviorSanitizer: invalid-bool-load` (on a few instances of seemingly correct code), adding `isCopyInstrImpl() -> std::nullopt` makes tests pass;
- llvm from 3cd3f11c174baa001b337b88c7a6507eb5705cf2 <https://reviews.llvm.org/rG3cd3f11c174baa001b337b88c7a6507eb5705cf2> (already includes b7836d856206ec39509d42529f958c920368166b <https://reviews.llvm.org/rGb7836d856206ec39509d42529f958c920368166b>) - tests fail exactly as in the case above, adding `isCopyInstrImpl() -> std::nullopt` makes tests pass.

The patch I used:

  --- llvm/include/llvm/CodeGen/TargetInstrInfo.h
  +++ llvm/include/llvm/CodeGen/TargetInstrInfo.h
  @@ -1039,11 +1039,11 @@
     /// target-dependent implementation.
     std::optional<DestSourcePair> isCopyInstr(const MachineInstr &MI) const {
       if (MI.isCopy()) {
         return DestSourcePair{MI.getOperand(0), MI.getOperand(1)};
       }
  -    return isCopyInstrImpl(MI);
  +    return std::nullopt;
     }
  
     bool isFullCopyInstr(const MachineInstr &MI) const {
       auto DestSrc = isCopyInstr(MI);
       if (!DestSrc)

Is this change something that can be committed to the tree? Do you need a better test case from us?

> If I step back and look at the original IR, it's branching on undef. From the entry in %bb, on the first branch to %bb.7, it's branching on undef here:
>
>   bb:
>      br label %bb7
>   
>   bb7:
>    ...
>     %i9 = phi i40 [ undef, %bb ], [ %i17, %bb5 ]
>     ...
>     %i13 = and i40 %i9, 4294967295
>     br i1 %i12, label %bb16, label %bb14
>
> Maybe this is just an artifact of reduction? If you try opt-bisect-limit, does it point at some other pass?

I'm almost sure it's an artifact of reduction, but it's also not trivial to create an interestingness test that completely avoids this sort of a degradation of the test case to something depending on UB.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150388