This is an archive of the discontinued LLVM Phabricator instance.

[peephole] Enhance folding logic to work for STATEPOINTs
ClosedPublic

Authored by reames on Aug 31 2016, 2:42 PM.

Details

Summary

The general idea here is to get enough of the existing restrictions out of the way that the already existing folding logic in foldMemoryOperand can kick in for STATEPOINTs and fold references to immutable stack slots. The key changes are:

  • Support for folding multiple operands at once which reference the same load
  • Support for folding multiple loads into a single instruction
  • Walk all the operands of the instruction for varidic instructions (this is a bug fix!)

Once this lands, I'll post another patch which refactors the TII interface here. There's nothing actually x86 specific about the x86 code used here.

Diff Detail

Event Timeline

reames updated this revision to Diff 69907.Aug 31 2016, 2:42 PM
reames retitled this revision from to [WIP][peephole] Enhance folding logic to work for STATEPOINTs.
reames updated this object.
reames added reviewers: lhames, qcolombet, sanjoy.
reames added a subscriber: llvm-commits.
reames added a reviewer: wmi.Sep 2 2016, 10:39 AM
wmi added inline comments.Sep 2 2016, 5:26 PM
lib/CodeGen/PeepholeOptimizer.cpp
1683

Why recursive folding is needed here? I guess it is trying to fold the case that the address of a load is another load? If that is true, can we put the use operands of newly folded load into a set and only visit the set in the next iteration?

lib/Target/X86/X86InstrInfo.cpp
5336

Looks like FoldAsLoadDefReg will never be 0 here.

5348

If MI is load, foldMemoryOperand will return nullptr in the end after it goes through a lot of effort. I agree with you it is better to remove the check here and add an early exit in foldMemoryOperand.

reames added inline comments.Sep 2 2016, 6:21 PM
lib/CodeGen/PeepholeOptimizer.cpp
1683

The case I'm trying to handle is a single instruction which can fold multiple distinct loads into it. The problem is that when I replace MI, I need to restart the operand iteration.

lib/Target/X86/X86InstrInfo.cpp
5336

That's what I thought, glad you agree.

5348

It will? I don't see why a instruction which can load definitely can't fold another load into itself. An instruction set with an "reg = add mem, mem" instruction would be a great example. Just because an "reg = add mem, reg" form loads doesn't mean you're done folding.

wmi added inline comments.Sep 2 2016, 11:27 PM
lib/CodeGen/PeepholeOptimizer.cpp
1683

I see. An idea is at the second iteration, don't have to restart the index i from the first use operand. we can start from "the last value of i in the previous iteration" + 1, i.e., keep i monotonically increasing from one iteration to the next, so the complexity is O(n).

lib/Target/X86/X86InstrInfo.cpp
5348

In X86InstrInfo::foldMemoryOperandImpl, if the opcode of current instruction is not in the table pointed by OpcodeTablePtr, it means it is impossible to get a valid instruction after folding, and foldMemoryOperandImpl will return nullptr. Like MemoryFoldTable1 specifies the set of instructions which can have their operand1 replaced by a mem operand.

reames updated this revision to Diff 70501.Sep 6 2016, 8:14 PM
reames retitled this revision from [WIP][peephole] Enhance folding logic to work for STATEPOINTs to [peephole] Enhance folding logic to work for STATEPOINTs.
reames updated this object.

A real patch now that we've settled the design questions.

wmi added inline comments.Sep 15 2016, 11:45 PM
lib/CodeGen/PeepholeOptimizer.cpp
1589

can we keep the continue and just let statepoint builtin to slip through? It is a little worrysome to let inlineasm or other instructions with sideeffect to try folding.

reames added inline comments.Oct 17 2016, 3:45 PM
lib/CodeGen/PeepholeOptimizer.cpp
1589

We won't fold across anything considered a isLoadFoldBarrier(). This is handled below. Is your concern that the folding logic might crash when folding a use into an instruction with side effects? If so, that would seem like a separate bug we should fix.

I am strongly resistant to special casing statepoint where generic code should work instead.

wmi accepted this revision.Oct 18 2016, 9:51 AM
wmi edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Oct 18 2016, 9:51 AM
This revision was automatically updated to reflect the committed changes.