[llvm] r175334 - [ms-inline asm] Do not omit the frame pointer if we have ms-inline assembly.
Benjamin Kramer
benny.kra at gmail.com
Mon Feb 18 11:47:21 PST 2013
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.
>
>> 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.
- 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