[llvm] r346718 - [MachineOutliner][NFC] Change getMachineOutlinerMBBFlags to isMBBSafeToOutlineFrom
Jessica Paquette via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 13 09:09:17 PST 2018
Hi Alexander,
Very sorry about the breakage! The fix sounds good to me. Thanks for looking at this.
- Jessica
> On Nov 13, 2018, at 8:44 AM, Alexander Kornienko <alexfh at google.com> wrote:
>
> I've commit the fix as r346761 to unblock us. If there's a better fix, feel free to apply it instead.
>
> On Tue, Nov 13, 2018 at 5:39 PM Alexander Kornienko <alexfh at google.com <mailto:alexfh at google.com>> wrote:
> On Tue, Nov 13, 2018 at 5:02 PM Alexander Kornienko <alexfh at google.com <mailto:alexfh at google.com>> wrote:
> On Tue, Nov 13, 2018 at 12:53 AM Jessica Paquette via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
> Author: paquette
> Date: Mon Nov 12 15:51:32 2018
> New Revision: 346718
>
> URL: http://llvm.org/viewvc/llvm-project?rev=346718&view=rev <http://llvm.org/viewvc/llvm-project?rev=346718&view=rev>
> Log:
> [MachineOutliner][NFC] Change getMachineOutlinerMBBFlags to isMBBSafeToOutlineFrom
>
> Instead of returning Flags, return true if the MBB is safe to outline from.
>
> This lets us check for unsafe situations, like say, in AArch64, X17 is live
> across a MBB without being defined in that MBB. In that case, there's no point
> in performing an instruction mapping.
>
> Modified:
> llvm/trunk/include/llvm/CodeGen/TargetInstrInfo.h
> llvm/trunk/lib/CodeGen/MachineOutliner.cpp
> llvm/trunk/lib/Target/AArch64/AArch64InstrInfo.cpp
> llvm/trunk/lib/Target/AArch64/AArch64InstrInfo.h
>
> Modified: llvm/trunk/include/llvm/CodeGen/TargetInstrInfo.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/TargetInstrInfo.h?rev=346718&r1=346717&r2=346718&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/TargetInstrInfo.h?rev=346718&r1=346717&r2=346718&view=diff>
> ==============================================================================
> --- llvm/trunk/include/llvm/CodeGen/TargetInstrInfo.h (original)
> +++ llvm/trunk/include/llvm/CodeGen/TargetInstrInfo.h Mon Nov 12 15:51:32 2018
> @@ -1635,10 +1635,11 @@ public:
> "Target didn't implement TargetInstrInfo::getOutliningType!");
> }
>
> - /// Returns target-defined flags defining properties of the MBB for
> - /// the outliner.
> - virtual unsigned getMachineOutlinerMBBFlags(MachineBasicBlock &MBB) const {
> - return 0x0;
> + /// Optional target hook that returns true if \p MBB is safe to outline from,
> + /// and returns any target-specific information in \p Flags.
> + virtual bool isMBBSafeToOutlineFrom(MachineBasicBlock &MBB,
> + unsigned &Flags) const {
> + return true;
>
> Should this return false?
>
> I guess, this should be fixed in a different way. See below.
>
> The way it is used in convertToUnsignedVec after this patch results in the Flags variable not being initialized and later used, which breaks test/CodeGen/AArch64/machine-outliner.mir. under memory sanitizer:
> MemorySanitizer: use-of-uninitialized-value
> #0 in llvm::AArch64InstrInfo::getOutliningType(llvm::MachineInstrBundleIterator<llvm::MachineInstr, false>&, unsigned int) const llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:5494:9
> #1 in (anonymous namespace)::InstructionMapper::convertToUnsignedVec(llvm::MachineBasicBlock&, llvm::TargetInstrInfo const&) llvm/lib/CodeGen/MachineOutliner.cpp:772:19
> #2 in (anonymous namespace)::MachineOutliner::populateMapper((anonymous namespace)::InstructionMapper&, llvm::Module&, llvm::MachineModuleInfo&) llvm/lib/CodeGen/MachineOutliner.cpp:1543:14
> #3 in (anonymous namespace)::MachineOutliner::runOnModule(llvm::Module&) llvm/lib/CodeGen/MachineOutliner.cpp:1645:3
> #4 in (anonymous namespace)::MPPassManager::runOnModule(llvm::Module&) llvm/lib/IR/LegacyPassManager.cpp:1744:27
> #5 in llvm::legacy::PassManagerImpl::run(llvm::Module&) llvm/lib/IR/LegacyPassManager.cpp:1857:44
> #6 in compileModule(char**, llvm::LLVMContext&) llvm/tools/llc/llc.cpp:597:8
>
> }
>
> /// Insert a custom frame for outlined functions.
>
> Modified: llvm/trunk/lib/CodeGen/MachineOutliner.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/MachineOutliner.cpp?rev=346718&r1=346717&r2=346718&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/MachineOutliner.cpp?rev=346718&r1=346717&r2=346718&view=diff>
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/MachineOutliner.cpp (original)
> +++ llvm/trunk/lib/CodeGen/MachineOutliner.cpp Mon Nov 12 15:51:32 2018
> @@ -742,7 +742,12 @@ struct InstructionMapper {
> /// \param TII \p TargetInstrInfo for the function.
> void convertToUnsignedVec(MachineBasicBlock &MBB,
> const TargetInstrInfo &TII) {
> - unsigned Flags = TII.getMachineOutlinerMBBFlags(MBB);
> + unsigned Flags;
>
> I suppose, the Flags still needs to be initialized, since both isMBBSafeToOutlineFrom implementations assume it's initialized.
>
> +
> + // Don't even map in this case.
> + if (!TII.isMBBSafeToOutlineFrom(MBB, Flags))
> + return;
> +
> MachineBasicBlock::iterator It = MBB.begin();
>
> // The number of instructions in this block that will be considered for
>
> Modified: llvm/trunk/lib/Target/AArch64/AArch64InstrInfo.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AArch64/AArch64InstrInfo.cpp?rev=346718&r1=346717&r2=346718&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AArch64/AArch64InstrInfo.cpp?rev=346718&r1=346717&r2=346718&view=diff>
> ==============================================================================
> --- llvm/trunk/lib/Target/AArch64/AArch64InstrInfo.cpp (original)
> +++ llvm/trunk/lib/Target/AArch64/AArch64InstrInfo.cpp Mon Nov 12 15:51:32 2018
> @@ -5286,29 +5286,44 @@ bool AArch64InstrInfo::isFunctionSafeToO
> return true;
> }
>
> -unsigned
> -AArch64InstrInfo::getMachineOutlinerMBBFlags(MachineBasicBlock &MBB) const {
> - unsigned Flags = 0x0;
> - // Check if there's a call inside this MachineBasicBlock. If there is, then
> - // set a flag.
> - if (any_of(MBB, [](MachineInstr &MI) { return MI.isCall(); }))
> - Flags |= MachineOutlinerMBBFlags::HasCalls;
> -
> +bool AArch64InstrInfo::isMBBSafeToOutlineFrom(MachineBasicBlock &MBB,
> + unsigned &Flags) const {
> // Check if LR is available through all of the MBB. If it's not, then set
> // a flag.
> assert(MBB.getParent()->getRegInfo().tracksLiveness() &&
> "Suitable Machine Function for outlining must track liveness");
> - LiveRegUnits LRU(getRegisterInfo());
> - LRU.addLiveOuts(MBB);
> + LiveRegUnits ModifiedRegUnits(getRegisterInfo());
> + LiveRegUnits UsedRegUnits(getRegisterInfo());
> + ModifiedRegUnits.addLiveOuts(MBB);
> + UsedRegUnits.addLiveOuts(MBB);
> + const TargetRegisterInfo *TRI = &getRegisterInfo();
> +
> + std::for_each(MBB.rbegin(), MBB.rend(),
> + [&ModifiedRegUnits, &UsedRegUnits, &TRI](MachineInstr &MI) {
> + LiveRegUnits::accumulateUsedDefed(MI, ModifiedRegUnits,
> + UsedRegUnits, TRI);
> + });
> +
> + // If one of these registers is live out of the MBB, but not modified in the
> + // MBB, then we can't outline.
> + if ((ModifiedRegUnits.available(AArch64::W16) &&
> + !UsedRegUnits.available(AArch64::W16)) ||
> + (ModifiedRegUnits.available(AArch64::W17) &&
> + !UsedRegUnits.available(AArch64::W17)) ||
> + (ModifiedRegUnits.available(AArch64::NZCV) &&
> + !UsedRegUnits.available(AArch64::NZCV)))
> + return false;
>
> - std::for_each(MBB.rbegin(),
> - MBB.rend(),
> - [&LRU](MachineInstr &MI) { LRU.accumulate(MI); });
> + // Check if there's a call inside this MachineBasicBlock. If there is, then
> + // set a flag.
> + if (any_of(MBB, [](MachineInstr &MI) { return MI.isCall(); }))
> + Flags |= MachineOutlinerMBBFlags::HasCalls;
>
> - if (!LRU.available(AArch64::LR))
> - Flags |= MachineOutlinerMBBFlags::LRUnavailableSomewhere;
> + if (!ModifiedRegUnits.available(AArch64::LR) ||
> + !UsedRegUnits.available(AArch64::LR))
> + Flags |= MachineOutlinerMBBFlags::LRUnavailableSomewhere;
>
> - return Flags;
> + return true;
> }
>
> outliner::InstrType
>
> Modified: llvm/trunk/lib/Target/AArch64/AArch64InstrInfo.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AArch64/AArch64InstrInfo.h?rev=346718&r1=346717&r2=346718&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AArch64/AArch64InstrInfo.h?rev=346718&r1=346717&r2=346718&view=diff>
> ==============================================================================
> --- llvm/trunk/lib/Target/AArch64/AArch64InstrInfo.h (original)
> +++ llvm/trunk/lib/Target/AArch64/AArch64InstrInfo.h Mon Nov 12 15:51:32 2018
> @@ -246,7 +246,8 @@ public:
> std::vector<outliner::Candidate> &RepeatedSequenceLocs) const override;
> outliner::InstrType
> getOutliningType(MachineBasicBlock::iterator &MIT, unsigned Flags) const override;
> - unsigned getMachineOutlinerMBBFlags(MachineBasicBlock &MBB) const override;
> + bool isMBBSafeToOutlineFrom(MachineBasicBlock &MBB,
> + unsigned &Flags) const override;
> void buildOutlinedFrame(MachineBasicBlock &MBB, MachineFunction &MF,
> const outliner::OutlinedFunction &OF) const override;
> MachineBasicBlock::iterator
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <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/20181113/2e6b113b/attachment.html>
More information about the llvm-commits
mailing list