[llvm] r175334 - [ms-inline asm] Do not omit the frame pointer if we have ms-inline assembly.
Chad Rosier
mcrosier at apple.com
Mon Feb 18 11:52:19 PST 2013
On Feb 18, 2013, at 11:47 AM, Benjamin Kramer <benny.kra at gmail.com> wrote:
>
> On 18.02.2013, at 20:25, Chad Rosier <mcrosier at apple.com> wrote:
>
>>
>> On Feb 17, 2013, at 6:05 AM, Benjamin Kramer <benny.kra at gmail.com> wrote:
>>
>>>
>>> On 16.02.2013, at 02:25, Chad Rosier <mcrosier at apple.com> wrote:
>>>
>>>> Author: mcrosier
>>>> Date: Fri Feb 15 19:25:28 2013
>>>> New Revision: 175334
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=175334&view=rev
>>>> Log:
>>>> [ms-inline asm] Do not omit the frame pointer if we have ms-inline assembly.
>>>>
>>>> If the frame pointer is omitted, and any stack changes occur in the inline
>>>> assembly, e.g.: "pusha", then any C local variable or C argument references
>>>> will be incorrect.
>>>
>>> Isn't this a X86-specific thing and should be handled with X86's setForceFramePointer?
>>
>> It is now, but the long-term plan is to support multiple ISAs, so I decided to use a target independent representation.
>>
>>> Feels weird to add yet another flag to MachineFunction for this.
>>
>> I don't disagree, so..
>>
>> If I were to change this to using the setForceFramePointer function, do you have a suggestion where I to call it?
>>
>> Lower formal arguments looks like a good place, but I don't want to have to iterate over the ISDAG. I don't believe the instruction have been lowered from ISD Nodes to MIs at that point, so I can't just iterate as was done in the original solution.
>>
>> Suggestions welcome.
>
> If we want to support multiple ISAs this point is moot, I'm just not sure if other ISAs will have the same frame pointer semantics (looks like a strange legacy quirk). OTOH I don't know how to handle it without another scan over the whole DAG so the current solution is not that bad.
Ok, sounds good.
>
>>
>>> This commit also caused a ton of valgrind failures: http://lab.llvm.org:8011/builders/llvm-x86_64-linux-vg_leak/builds/328
>>
>> I believe this was fixed in r175422.
>
> Yeah, that should work, the bot just can't cycle back to green because its disk is full.
Great.
Chad
>
> - Ben
>>
>> Chad
>>
>>>
>>> Backtrace looks like this:
>>> ==30816== Conditional jump or move depends on uninitialised value(s)
>>> ==30816== at 0xCCC440: llvm::X86FrameLowering::hasFP(llvm::MachineFunction const&) const (X86FrameLowering.cpp:55)
>>> ==30816== by 0xCBA08A: llvm::X86RegisterInfo::getRegPressureLimit(llvm::TargetRegisterClass const*, llvm::MachineFunction&) const (X86RegisterInfo.cpp:221)
>>> ==30816== by 0xE2D715: (anonymous namespace)::RegReductionPQBase::RegReductionPQBase(llvm::MachineFunction&, bool, bool, bool, llvm::TargetInstrInfo const*, llvm::TargetRegisterInfo const*, llvm::TargetLowering const*) (ScheduleDAGRRList.cpp:1639)
>>> ==30816== by 0xE32305: (anonymous namespace)::RegReductionPriorityQueue<(anonymous namespace)::ilp_ls_rr_sort>::RegReductionPriorityQueue(llvm::MachineFunction&, bool, bool, llvm::TargetInstrInfo const*, llvm::TargetRegisterInfo const*, llvm::TargetLowering const*) (ScheduleDAGRRList.cpp:1750)
>>> ==30816== by 0xE3201F: llvm::createILPListDAGScheduler(llvm::SelectionDAGISel*, llvm::CodeGenOpt::Level) (ScheduleDAGRRList.cpp:3008)
>>> ==30816== by 0xEC9C59: llvm::createDefaultScheduler(llvm::SelectionDAGISel*, llvm::CodeGenOpt::Level) (SelectionDAGISel.cpp:234)
>>> ==30816== by 0xECF887: llvm::SelectionDAGISel::CreateScheduler() (SelectionDAGISel.cpp:1410)
>>> ==30816== by 0xECC413: llvm::SelectionDAGISel::CodeGenAndEmitDAG() (SelectionDAGISel.cpp:683)
>>> ==30816== by 0xECB1E5: llvm::SelectionDAGISel::SelectBasicBlock(llvm::ilist_iterator<llvm::Instruction const>, llvm::ilist_iterator<llvm::Instruction const>, bool&) (SelectionDAGISel.cpp:515)
>>> ==30816== by 0xECE032: llvm::SelectionDAGISel::SelectAllBasicBlocks(llvm::Function const&) (SelectionDAGISel.cpp:1184)
>>> ==30816== by 0xECA56C: llvm::SelectionDAGISel::runOnMachineFunction(llvm::MachineFunction&) (SelectionDAGISel.cpp:375)
>>> ==30816== by 0x10A7F10: llvm::MachineFunctionPass::runOnFunction(llvm::Function&) (MachineFunctionPass.cpp:33)
>>> ==30816==
>>>
>>>
>>>>
>>>> I pass no judgement on anyone who would do such a thing. ;)
>>>> rdar://13218191
>>>>
>>>> Modified:
>>>> llvm/trunk/include/llvm/CodeGen/MachineFunction.h
>>>> llvm/trunk/include/llvm/CodeGen/MachineInstr.h
>>>> llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
>>>> llvm/trunk/lib/Target/X86/X86FrameLowering.cpp
>>>> llvm/trunk/test/CodeGen/X86/ms-inline-asm.ll
>>>>
>>>> Modified: llvm/trunk/include/llvm/CodeGen/MachineFunction.h
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/MachineFunction.h?rev=175334&r1=175333&r2=175334&view=diff
>>>> ==============================================================================
>>>> --- llvm/trunk/include/llvm/CodeGen/MachineFunction.h (original)
>>>> +++ llvm/trunk/include/llvm/CodeGen/MachineFunction.h Fri Feb 15 19:25:28 2013
>>>> @@ -131,6 +131,9 @@ class MachineFunction {
>>>> /// about the control flow of such functions.
>>>> bool ExposesReturnsTwice;
>>>>
>>>> + /// True if the function includes MS-style inline assembly.
>>>> + bool HasMSInlineAsm;
>>>> +
>>>> MachineFunction(const MachineFunction &) LLVM_DELETED_FUNCTION;
>>>> void operator=(const MachineFunction&) LLVM_DELETED_FUNCTION;
>>>> public:
>>>> @@ -214,6 +217,17 @@ public:
>>>> void setExposesReturnsTwice(bool B) {
>>>> ExposesReturnsTwice = B;
>>>> }
>>>> +
>>>> + /// Returns true if the function contains any MS-style inline assembly.
>>>> + bool hasMSInlineAsm() const {
>>>> + return HasMSInlineAsm;
>>>> + }
>>>> +
>>>> + /// Set a flag that indicates that the function contains MS-style inline
>>>> + /// assembly.
>>>> + void setHasMSInlineAsm(bool B) {
>>>> + HasMSInlineAsm = B;
>>>> + }
>>>>
>>>> /// getInfo - Keep track of various per-function pieces of information for
>>>> /// backends that would like to do so.
>>>>
>>>> Modified: llvm/trunk/include/llvm/CodeGen/MachineInstr.h
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/MachineInstr.h?rev=175334&r1=175333&r2=175334&view=diff
>>>> ==============================================================================
>>>> --- llvm/trunk/include/llvm/CodeGen/MachineInstr.h (original)
>>>> +++ llvm/trunk/include/llvm/CodeGen/MachineInstr.h Fri Feb 15 19:25:28 2013
>>>> @@ -642,6 +642,9 @@ public:
>>>> bool isKill() const { return getOpcode() == TargetOpcode::KILL; }
>>>> bool isImplicitDef() const { return getOpcode()==TargetOpcode::IMPLICIT_DEF; }
>>>> bool isInlineAsm() const { return getOpcode() == TargetOpcode::INLINEASM; }
>>>> + bool isMSInlineAsm() const {
>>>> + return getOpcode() == TargetOpcode::INLINEASM && getInlineAsmDialect();
>>>> + }
>>>> bool isStackAligningInlineAsm() const;
>>>> InlineAsm::AsmDialect getInlineAsmDialect() const;
>>>> bool isInsertSubreg() const {
>>>>
>>>> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp?rev=175334&r1=175333&r2=175334&view=diff
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp (original)
>>>> +++ llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp Fri Feb 15 19:25:28 2013
>>>> @@ -441,25 +441,28 @@ bool SelectionDAGISel::runOnMachineFunct
>>>> }
>>>>
>>>> // Determine if there are any calls in this machine function.
>>>> + MF->setHasMSInlineAsm(false);
>>>> MachineFrameInfo *MFI = MF->getFrameInfo();
>>>> - if (!MFI->hasCalls()) {
>>>> - for (MachineFunction::const_iterator
>>>> - I = MF->begin(), E = MF->end(); I != E; ++I) {
>>>> - const MachineBasicBlock *MBB = I;
>>>> - for (MachineBasicBlock::const_iterator
>>>> - II = MBB->begin(), IE = MBB->end(); II != IE; ++II) {
>>>> - const MCInstrDesc &MCID = TM.getInstrInfo()->get(II->getOpcode());
>>>> + for (MachineFunction::const_iterator I = MF->begin(), E = MF->end(); I != E;
>>>> + ++I) {
>>>>
>>>> - if ((MCID.isCall() && !MCID.isReturn()) ||
>>>> - II->isStackAligningInlineAsm()) {
>>>> - MFI->setHasCalls(true);
>>>> - goto done;
>>>> - }
>>>> + if (MFI->hasCalls() && MF->hasMSInlineAsm())
>>>> + break;
>>>> +
>>>> + const MachineBasicBlock *MBB = I;
>>>> + for (MachineBasicBlock::const_iterator II = MBB->begin(), IE = MBB->end();
>>>> + II != IE; ++II) {
>>>> + const MCInstrDesc &MCID = TM.getInstrInfo()->get(II->getOpcode());
>>>> + if ((MCID.isCall() && !MCID.isReturn()) ||
>>>> + II->isStackAligningInlineAsm()) {
>>>> + MFI->setHasCalls(true);
>>>> + }
>>>> + if (II->isMSInlineAsm()) {
>>>> + MF->setHasMSInlineAsm(true);
>>>> }
>>>> }
>>>> }
>>>>
>>>> - done:
>>>> // Determine if there is a call to setjmp in the machine function.
>>>> MF->setExposesReturnsTwice(Fn.callsFunctionThatReturnsTwice());
>>>>
>>>>
>>>> Modified: llvm/trunk/lib/Target/X86/X86FrameLowering.cpp
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86FrameLowering.cpp?rev=175334&r1=175333&r2=175334&view=diff
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/Target/X86/X86FrameLowering.cpp (original)
>>>> +++ llvm/trunk/lib/Target/X86/X86FrameLowering.cpp Fri Feb 15 19:25:28 2013
>>>> @@ -50,7 +50,7 @@ bool X86FrameLowering::hasFP(const Machi
>>>> return (MF.getTarget().Options.DisableFramePointerElim(MF) ||
>>>> RegInfo->needsStackRealignment(MF) ||
>>>> MFI->hasVarSizedObjects() ||
>>>> - MFI->isFrameAddressTaken() ||
>>>> + MFI->isFrameAddressTaken() || MF.hasMSInlineAsm() ||
>>>> MF.getInfo<X86MachineFunctionInfo>()->getForceFramePointer() ||
>>>> MMI.callsUnwindInit() || MMI.callsEHReturn());
>>>> }
>>>>
>>>> Modified: llvm/trunk/test/CodeGen/X86/ms-inline-asm.ll
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/ms-inline-asm.ll?rev=175334&r1=175333&r2=175334&view=diff
>>>> ==============================================================================
>>>> --- llvm/trunk/test/CodeGen/X86/ms-inline-asm.ll (original)
>>>> +++ llvm/trunk/test/CodeGen/X86/ms-inline-asm.ll Fri Feb 15 19:25:28 2013
>>>> @@ -5,6 +5,7 @@ entry:
>>>> %0 = tail call i32 asm sideeffect inteldialect "mov eax, $1\0A\09mov $0, eax", "=r,r,~{eax},~{dirflag},~{fpsr},~{flags}"(i32 1) nounwind
>>>> ret i32 %0
>>>> ; CHECK: t1
>>>> +; CHECK: movl %esp, %ebp
>>>> ; CHECK: {{## InlineAsm Start|#APP}}
>>>> ; CHECK: .intel_syntax
>>>> ; CHECK: mov eax, ecx
>>>> @@ -18,6 +19,7 @@ entry:
>>>> call void asm sideeffect inteldialect "mov eax, $$1", "~{eax},~{dirflag},~{fpsr},~{flags}"() nounwind
>>>> ret void
>>>> ; CHECK: t2
>>>> +; CHECK: movl %esp, %ebp
>>>> ; CHECK: {{## InlineAsm Start|#APP}}
>>>> ; CHECK: .intel_syntax
>>>> ; CHECK: mov eax, 1
>>>> @@ -32,6 +34,7 @@ entry:
>>>> call void asm sideeffect inteldialect "mov eax, DWORD PTR [$0]", "*m,~{eax},~{dirflag},~{fpsr},~{flags}"(i32* %V.addr) nounwind
>>>> ret void
>>>> ; CHECK: t3
>>>> +; CHECK: movl %esp, %ebp
>>>> ; CHECK: {{## InlineAsm Start|#APP}}
>>>> ; CHECK: .intel_syntax
>>>> ; CHECK: mov eax, DWORD PTR {{[[esp]}}
>>>> @@ -53,6 +56,7 @@ entry:
>>>> %0 = load i32* %b1, align 4
>>>> ret i32 %0
>>>> ; CHECK: t18
>>>> +; CHECK: movl %esp, %ebp
>>>> ; CHECK: {{## InlineAsm Start|#APP}}
>>>> ; CHECK: .intel_syntax
>>>> ; CHECK: lea ebx, foo
>>>> @@ -72,6 +76,7 @@ entry:
>>>> call void asm sideeffect inteldialect "call $0", "r,~{dirflag},~{fpsr},~{flags}"(void ()* @t19_helper) nounwind
>>>> ret void
>>>> ; CHECK: t19:
>>>> +; CHECK: movl %esp, %ebp
>>>> ; CHECK: movl ${{_?}}t19_helper, %eax
>>>> ; CHECK: {{## InlineAsm Start|#APP}}
>>>> ; CHECK: .intel_syntax
>>>> @@ -90,6 +95,7 @@ entry:
>>>> %0 = load i32** %res, align 4
>>>> ret i32* %0
>>>> ; CHECK: t30:
>>>> +; CHECK: movl %esp, %ebp
>>>> ; CHECK: {{## InlineAsm Start|#APP}}
>>>> ; CHECK: .intel_syntax
>>>> ; CHECK: lea edi, dword ptr [{{_?}}results]
>>>> @@ -97,8 +103,8 @@ entry:
>>>> ; CHECK: {{## InlineAsm End|#NO_APP}}
>>>> ; CHECK: {{## InlineAsm Start|#APP}}
>>>> ; CHECK: .intel_syntax
>>>> -; CHECK: mov dword ptr [esp], edi
>>>> +; CHECK: mov dword ptr [ebp - 8], edi
>>>> ; CHECK: .att_syntax
>>>> ; CHECK: {{## InlineAsm End|#NO_APP}}
>>>> -; CHECK: movl (%esp), %eax
>>>> +; CHECK: movl -8(%ebp), %eax
>>>> }
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>
>
More information about the llvm-commits
mailing list