[llvm] r291973 - [GlobalISel] track predecessor mapping during switch lowering.
Daniel Jasper via llvm-commits
llvm-commits at lists.llvm.org
Sun Jan 15 01:53:08 PST 2017
This fails in a cmake Release build with LLVM_BUILD_GLOBAL_ISEL enabled (on
x86):
********************
FAIL: LLVM :: CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll (3392 of
19335)
******************** TEST 'LLVM ::
CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll' FAILED
********************
Script:
--
/home/djasper/llvm/release/./bin/llc -O0 -aarch64-enable-atomic-cfg-tidy=0
-stop-after=irtranslator -global-isel -verify-machineinstrs
/home/djasper/llvm/test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll -o
- 2>&1 | /home/djasper/llvm/release/./bin/FileCheck
/home/djasper/llvm/test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll
--
Exit Code: 1
Command Output (stderr):
--
/home/djasper/llvm/test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll:9:16:
error: expected string not found in input
; CHECK-LABEL: name: addi64
^
<stdin>:1:1: note: scanning from here
llc: /home/djasper/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:762: void
llvm::IRTranslator::finishPendingPhis(): Assertion
`Pred->isSuccessor(MIB->getParent()) && "incorrect CFG at MachineBasicBlock
level"' failed.
^
<stdin>:14:66: note: possible intended match here
#12 0x00000000018c3ff6
llvm::IRTranslator::runOnMachineFunction(llvm::MachineFunction&)
/home/djasper/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:882:30
^
--
********************
If I remove the FileCheck, then it shows that llc crashes.
For now, I have reverted this in r292061. Please let me know if there is
anything more I can do to help.
Cheers,
Daniel
On Sat, Jan 14, 2017 at 12:11 AM, Tim Northover via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
> Author: tnorthover
> Date: Fri Jan 13 17:11:37 2017
> New Revision: 291973
>
> URL: http://llvm.org/viewvc/llvm-project?rev=291973&view=rev
> Log:
> [GlobalISel] track predecessor mapping during switch lowering.
>
> Correctly populating Machine PHIs relies on knowing exactly how the IR
> level
> CFG was lowered to MachineIR. This needs to be tracked by any translation
> phases that meddle (currently only SwitchInst handling).
>
> Modified:
> llvm/trunk/include/llvm/CodeGen/GlobalISel/IRTranslator.h
> llvm/trunk/lib/CodeGen/GlobalISel/IRTranslator.cpp
> llvm/trunk/test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll
>
> Modified: llvm/trunk/include/llvm/CodeGen/GlobalISel/IRTranslator.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/
> llvm/CodeGen/GlobalISel/IRTranslator.h?rev=291973&r1=
> 291972&r2=291973&view=diff
> ============================================================
> ==================
> --- llvm/trunk/include/llvm/CodeGen/GlobalISel/IRTranslator.h (original)
> +++ llvm/trunk/include/llvm/CodeGen/GlobalISel/IRTranslator.h Fri Jan 13
> 17:11:37 2017
> @@ -70,6 +70,9 @@ private:
> // lives.
> DenseMap<const BasicBlock *, MachineBasicBlock *> BBToMBB;
>
> + typedef std::pair<const BasicBlock *, const BasicBlock *> CFGEdge;
> + DenseMap<CFGEdge, SmallVector<MachineBasicBlock *, 1>> MachinePreds;
> +
> // List of stubbed PHI instructions, for values and basic blocks to be
> filled
> // in once all MachineBasicBlocks have been created.
> SmallVector<std::pair<const PHINode *, MachineInstr *>, 4> PendingPHIs;
> @@ -390,10 +393,27 @@ private:
> /// the type being accessed (according to the Module's DataLayout).
> unsigned getMemOpAlignment(const Instruction &I);
>
> - /// Get the MachineBasicBlock that represents \p BB.
> - /// If such basic block does not exist, it is created.
> + /// Get the MachineBasicBlock that represents \p BB. Specifically, the
> block
> + /// returned will be the head of the translated block (suitable for
> branch
> + /// destinations). If such basic block does not exist, it is created.
> MachineBasicBlock &getOrCreateBB(const BasicBlock &BB);
>
> + /// Record \p NewPred as a Machine predecessor to `Edge.second`,
> corresponding
> + /// to `Edge.first` at the IR level. This is used when IRTranslation
> creates
> + /// multiple MachineBasicBlocks for a given IR block and the CFG is no
> longer
> + /// represented simply by the IR-level CFG.
> + void addMachineCFGPred(CFGEdge Edge, MachineBasicBlock *NewPred);
> +
> + /// Returns the Machine IR predecessors for the given IR CFG edge.
> Usually
> + /// this is just the single MachineBasicBlock corresponding to the
> predecessor
> + /// in the IR. More complex lowering can result in multiple
> MachineBasicBlocks
> + /// preceding the original though (e.g. switch instructions).
> + ArrayRef<MachineBasicBlock *> getMachinePredBBs(CFGEdge Edge) {
> + auto RemappedEdge = MachinePreds.find(Edge);
> + if (RemappedEdge != MachinePreds.end())
> + return RemappedEdge->second;
> + return &getOrCreateBB(*Edge.first);
> + }
>
> public:
> // Ctor, nothing fancy.
>
> Modified: llvm/trunk/lib/CodeGen/GlobalISel/IRTranslator.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/
> CodeGen/GlobalISel/IRTranslator.cpp?rev=291973&
> r1=291972&r2=291973&view=diff
> ============================================================
> ==================
> --- llvm/trunk/lib/CodeGen/GlobalISel/IRTranslator.cpp (original)
> +++ llvm/trunk/lib/CodeGen/GlobalISel/IRTranslator.cpp Fri Jan 13
> 17:11:37 2017
> @@ -12,6 +12,7 @@
>
> #include "llvm/CodeGen/GlobalISel/IRTranslator.h"
>
> +#include "llvm/ADT/SmallSet.h"
> #include "llvm/ADT/SmallVector.h"
> #include "llvm/CodeGen/GlobalISel/CallLowering.h"
> #include "llvm/CodeGen/Analysis.h"
> @@ -134,6 +135,11 @@ MachineBasicBlock &IRTranslator::getOrCr
> return *MBB;
> }
>
> +void IRTranslator::addMachineCFGPred(CFGEdge Edge, MachineBasicBlock
> *NewPred) {
> + assert(NewPred && "new edge must be a real MachineBasicBlock");
> + MachinePreds[Edge].push_back(NewPred);
> +}
> +
> bool IRTranslator::translateBinaryOp(unsigned Opcode, const User &U,
> MachineIRBuilder &MIRBuilder) {
> // FIXME: handle signed/unsigned wrapping flags.
> @@ -209,30 +215,36 @@ bool IRTranslator::translateSwitch(const
>
> const SwitchInst &SwInst = cast<SwitchInst>(U);
> const unsigned SwCondValue = getOrCreateVReg(*SwInst.getCondition());
> + const BasicBlock *OrigBB = SwInst.getParent();
>
> LLT LLTi1 = LLT(*Type::getInt1Ty(U.getContext()), *DL);
> for (auto &CaseIt : SwInst.cases()) {
> const unsigned CaseValueReg = getOrCreateVReg(*CaseIt.
> getCaseValue());
> const unsigned Tst = MRI->createGenericVirtualRegister(LLTi1);
> MIRBuilder.buildICmp(CmpInst::ICMP_EQ, Tst, CaseValueReg,
> SwCondValue);
> - MachineBasicBlock &CurBB = MIRBuilder.getMBB();
> - MachineBasicBlock &TrueBB = getOrCreateBB(*CaseIt.
> getCaseSuccessor());
> -
> - MIRBuilder.buildBrCond(Tst, TrueBB);
> - CurBB.addSuccessor(&TrueBB);
> + MachineBasicBlock &CurMBB = MIRBuilder.getMBB();
> + const BasicBlock *TrueBB = CaseIt.getCaseSuccessor();
> + MachineBasicBlock &TrueMBB = getOrCreateBB(*TrueBB);
> +
> + MIRBuilder.buildBrCond(Tst, TrueMBB);
> + CurMBB.addSuccessor(&TrueMBB);
> + addMachineCFGPred({OrigBB, TrueBB}, &CurMBB);
>
> - MachineBasicBlock *FalseBB =
> + MachineBasicBlock *FalseMBB =
> MF->CreateMachineBasicBlock(SwInst.getParent());
> - MF->push_back(FalseBB);
> - MIRBuilder.buildBr(*FalseBB);
> - CurBB.addSuccessor(FalseBB);
> + MF->push_back(FalseMBB);
> + MIRBuilder.buildBr(*FalseMBB);
> + CurMBB.addSuccessor(FalseMBB);
>
> - MIRBuilder.setMBB(*FalseBB);
> + MIRBuilder.setMBB(*FalseMBB);
> }
> // handle default case
> - MachineBasicBlock &DefaultBB = getOrCreateBB(*SwInst.getDefaultDest());
> - MIRBuilder.buildBr(DefaultBB);
> - MIRBuilder.getMBB().addSuccessor(&DefaultBB);
> + const BasicBlock *DefaultBB = SwInst.getDefaultDest();
> + MachineBasicBlock &DefaultMBB = getOrCreateBB(*DefaultBB);
> + MIRBuilder.buildBr(DefaultMBB);
> + MachineBasicBlock &CurMBB = MIRBuilder.getMBB();
> + CurMBB.addSuccessor(&DefaultMBB);
> + addMachineCFGPred({OrigBB, DefaultBB}, &CurMBB);
>
> return true;
> }
> @@ -736,11 +748,21 @@ void IRTranslator::finishPendingPhis() {
> // won't create extra control flow here, otherwise we need to find the
> // dominating predecessor here (or perhaps force the weirder
> IRTranslators
> // to provide a simple boundary).
> + SmallSet<const BasicBlock *, 4> HandledPreds;
> +
> for (unsigned i = 0; i < PI->getNumIncomingValues(); ++i) {
> - assert(BBToMBB[PI->getIncomingBlock(i)]->isSuccessor(MIB->getParent())
> &&
> - "I appear to have misunderstood Machine PHIs");
> - MIB.addUse(getOrCreateVReg(*PI->getIncomingValue(i)));
> - MIB.addMBB(BBToMBB[PI->getIncomingBlock(i)]);
> + auto IRPred = PI->getIncomingBlock(i);
> + if (HandledPreds.count(IRPred))
> + continue;
> +
> + HandledPreds.insert(IRPred);
> + unsigned ValReg = getOrCreateVReg(*PI->getIncomingValue(i));
> + for (auto Pred : getMachinePredBBs({IRPred, PI->getParent()})) {
> + assert(Pred->isSuccessor(MIB->getParent()) &&
> + "incorrect CFG at MachineBasicBlock level");
> + MIB.addUse(ValReg);
> + MIB.addMBB(Pred);
> + }
> }
> }
> }
> @@ -794,6 +816,7 @@ void IRTranslator::finalizeFunction() {
> ValToVReg.clear();
> FrameIndices.clear();
> Constants.clear();
> + MachinePreds.clear();
> }
>
> bool IRTranslator::runOnMachineFunction(MachineFunction &CurMF) {
>
> Modified: llvm/trunk/test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/
> CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll?rev=
> 291973&r1=291972&r2=291973&view=diff
> ============================================================
> ==================
> --- llvm/trunk/test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll
> (original)
> +++ llvm/trunk/test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll Fri
> Jan 13 17:11:37 2017
> @@ -168,6 +168,55 @@ return:
> ret i32 %res
> }
>
> + ; The switch lowering code changes the CFG, which means that the
> original
> + ; %entry block is no longer a predecessor for the phi instruction. We
> need to
> + ; use the correct lowered MachineBasicBlock instead.
> +; CHECK-LABEL: name: test_cfg_remap
> +
> +; CHECK: bb.5.entry:
> +; CHECK-NEXT: successors: %[[PHI_BLOCK:bb.[0-9]+.phi.block]]
> +; CHECK: G_BR %[[PHI_BLOCK]]
> +
> +; CHECK: [[PHI_BLOCK]]:
> +; CHECK-NEXT: PHI %{{.*}}(s32), %bb.5.entry
> +define i32 @test_cfg_remap(i32 %in) {
> +entry:
> + switch i32 %in, label %phi.block [i32 1, label %next
> + i32 57, label %other]
> +
> +next:
> + br label %phi.block
> +
> +other:
> + ret i32 undef
> +
> +phi.block:
> + %res = phi i32 [1, %entry], [42, %next]
> + ret i32 %res
> +}
> +
> +; CHECK-LABEL: name: test_cfg_remap_multiple_preds
> +; CHECK: PHI [[ENTRY:%.*]](s32), %bb.{{[0-9]+}}.entry, [[ENTRY]](s32),
> %bb.{{[0-9]+}}.entry
> +define i32 @test_cfg_remap_multiple_preds(i32 %in) {
> +entry:
> + switch i32 %in, label %odd [i32 1, label %next
> + i32 57, label %other
> + i32 128, label %phi.block
> + i32 256, label %phi.block]
> +odd:
> + unreachable
> +
> +next:
> + br label %phi.block
> +
> +other:
> + ret i32 undef
> +
> +phi.block:
> + %res = phi i32 [1, %entry], [1, %entry], [42, %next]
> + ret i32 12
> +}
> +
> ; Tests for or.
> ; CHECK-LABEL: name: ori64
> ; CHECK: [[ARG1:%[0-9]+]](s64) = COPY %x0
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170115/59de6496/attachment.html>
More information about the llvm-commits
mailing list