nickdesaulniers accepted this revision. nickdesaulniers added a comment. This revision is now accepted and ready to land.
This code has now been tested on a running Linux kernel making use of the feature. I still would like @jyknight to clarify his comments, consider explicitly requesting changes to this CL, or consider resigning as reviewer. ================ Comment at: llvm/include/llvm/CodeGen/MachineBasicBlock.h:137 + /// List of indirect targets of the callbr of a basic block. + SmallPtrSet<const MachineBasicBlock*, 4> InlineAsmBrIndirectTargets; + ---------------- It's likely the count here is 0, or maybe 1. We don't see too often a large list of labels here. ================ Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:701 + } else if (MBB->succ_size() == LandingPadSuccs.size() || + MBB->succ_size() == IndirectPadSuccs.size()) { // It's possible that the block legitimately ends with a noreturn ---------------- void wrote: > void wrote: > > jyknight wrote: > > > void wrote: > > > > jyknight wrote: > > > > > This isn't correct. > > > > > > > > > > This line here, is looking at a block which doesn't end in a jump to > > > > > a successor. So, it's trying to verify that the successor list makes > > > > > sense in that context. > > > > > > > > > > The unstated assumption in the code is that the only successors will > > > > > be landing pads. Instead of actually checking each one, instead it > > > > > just checks that the count is the number of landing pads, with the > > > > > assumption that all the successors should be landing pads, and that > > > > > all the landing pads should be successors. > > > > > > > > > > The next clause is then checking for the case where there's a > > > > > fallthrough to the next block. In that case, the successors should've > > > > > been all the landing pads, and the single fallthrough block. > > > > > > > > > > Adding similar code to check for the number of callbr targets doesn't > > > > > really make sense. It's certainly not the case that all callbr > > > > > targets are targets of all > > > > > callbr instructions. And even if it was, this still wouldn't be > > > > > counting things correctly. > > > > > > > > > > > > > > > However -- I think i'd expect analyzeBranch to error out (returning > > > > > true) when confronted by a callbr instruction, because it cannot > > > > > actually tell what's going on there. If that were the case, nothing > > > > > in this block should even be invoked. But I guess that's probably not > > > > > happening, due to the terminator being followed by non-terminators. > > > > > > > > > > That seems likely to be a problem that needs to be fixed. (And if > > > > > that is fixed, I think the changes here aren't needed anymore) > > > > > > > > > > > > > > Your comment is very confusing. Could you please give an example of > > > > where this fails? > > > Sorry about that, I should've delimited the parts of that message > > > better... > > > Basically: > > > - Paragraphs 2-4 are describing why the code before this patch appears to > > > be correct for landing pad, even though it's taking some shortcuts and > > > making some non-obvious assumptions. > > > - Paragraph 5 ("Adding similar code"...) is why it's not correct for > > > callbr. > > > - Paragraph 6-7 are how I'd suggest to resolve it. > > > > > > > > > I believe the code as of your patch will fail validation if you have a > > > callbr instruction which has a normal-successor block which is an > > > indirect target of a *different* callbr in the function. > > > > > > I believe it'll also fail if you have any landing-pad successors, since > > > those aren't being added to the count of expected successors, but rather > > > checked separately. > > > > > > But more seriously than these potential verifier failures, I expect that > > > analyzeBranch returning wrong answers (in that it may report that a block > > > unconditionally-jumps to a successor, while it really has both a callbr > > > and jump, separated by the non-terminator copies) will cause > > > miscompilation. I'm not sure exactly how that will exhibit, but I'm > > > pretty sure it's not going to be good. > > > > > > And, if analyzeBranch properly said "no idea" when confronted by callbr > > > control flow, then this code in the verifier wouldn't be reached. > > I didn't need a delineation of the parts of the comment. I needed a clearer > > description of what your concern is, and to give an example of code that > > fails here. > > > > This bit of code is simply saying that if the block containing the > > `INLINEASM_BR` doesn't end with a `BR` instruction, then the number of its > > successors should be equal to the number of indirect successors. This is > > correct, as it's not valid to have a duplicate label used in a `callbr` > > instruction: > > > > ``` > > $ llc -o /dev/null x.ll > > Duplicate callbr destination! > > %3 = callbr i32 asm sideeffect "testl $0, $0; testl $1, $1; jne ${2:l}", > > "={si},r,X,0,~{dirflag},~{fpsr},~{flags}"(i32 %2, i8* blockaddress(@test1, > > %asm.fallthrough), i32 %1) #2 > > to label %asm.fallthrough [label %asm.fallthrough], !srcloc !6 > > ./bin/llc: x.ll: error: input module is broken! > > ``` > > > > A `callbr` with a normal successor block that is the indirect target of a > > different `callbr` isn't really relevant here, unless I'm misunderstanding > > what `analyzeBranch` returns. There would be two situations: > > > > 1. The MBB ends in a fallthrough, which is the case I mentioned above, or > > > > 2. The MBB ends in a `BR` instruction, in which case it won't be in this > > block of code, but the block below. > > > > If `analyzeBranch` is not taking into account potential `COPY` instructions > > between `INLINEASM_BR` and `BR`, then it needs to be addressed there (I'll > > verify that it is). I *do* know that this code is reached by the verifier, > > so it handles it to some degree. :-) > > But more seriously than these potential verifier failures, I expect that > > analyzeBranch returning wrong answers (in that it may report that a block > > unconditionally-jumps to a successor, while it really has both a callbr and > > jump, separated by the non-terminator copies) will cause miscompilation. > > I'm > > not sure exactly how that will exhibit, but I'm pretty sure it's not going > > to be good. > > Here are two proposals that may help alleviate these concerns: > > 1. Have analyzeBranch skip over the COPYs between the INLINEASM_BR and the > JMP. It's relatively straight-forward to do, but it would have to be done for > *all* analyzeBranch calls. > > 2. Create a new pseudo-instruction called `INLINEASM_BR_COPY` (or some better > name) that's a terminator which behaves like a normal `COPY`, but the > analyzeBranch and other methods that look at terminators will be able to > handle it without modifications, since it'll look similarly to an > `INLINEASM_BR` instruction. It doesn't require changing all of analyzeBranch > implementations, but it's a much larger change. > > Thoughts? > Instead of actually checking each one, instead it just checks that the count > is the number of landing pads, with the assumption that all the successors > should be landing pads, and that all the landing pads should be successors. What do you mean "instead of actually checking each one?" What check should be done? > It's certainly not the case that all callbr targets are targets of all callbr > instructions. And even if it was, this still wouldn't be counting things > correctly. Right, so you could have a `MachineBasicBlock` that's the target of a `INLINEASM_BR`, and a different `MachineBasicBlock` that also branches to the `INLINEASM_BR` target (but itself wasn't an `INLINEASM_BR`). But `IndirectTargetSuccs` is only built up from successors of the current block that are `isInlineAsmBrIndirectTarget`'s. So I don't understand how the count is "wrong" just because you could have other MBB's also target the current MBB's indirect successor. ================ Comment at: llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp:1076 + // to the copy so that everyone is happy. + for (auto *Succ : BB->successors()) + if (Succ != CopyBB && !CopyBB->isSuccessor(Succ)) ---------------- Isn't `Fallthrough` from above one of the potential successors? Do we have to skip it in the below conditional? What happens if we call `addSuccessor` with the same `MBB` twice? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69868/new/ https://reviews.llvm.org/D69868 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits