<div dir="ltr">CL is casing <a href="http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/13776/steps/check-llvm%20msan/logs/stdio">http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/13776/steps/check-llvm%20msan/logs/stdio</a></div><br><div class="gmail_quote"><div dir="ltr">On Wed, Jun 15, 2016 at 9:47 AM Kevin B. Smith via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: kbsmith1<br>
Date: Wed Jun 15 11:03:06 2016<br>
New Revision: 272797<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=272797&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=272797&view=rev</a><br>
Log:<br>
[X86]: Improve Liveness checking for X86FixupBWInsts.cpp<br>
Differential Revision: <a href="http://reviews.llvm.org/D21085" rel="noreferrer" target="_blank">http://reviews.llvm.org/D21085</a><br>
<br>
Added:<br>
llvm/trunk/test/CodeGen/X86/fixup-bw-inst-fwlive.mir<br>
Modified:<br>
llvm/trunk/lib/Target/X86/X86FixupBWInsts.cpp<br>
<br>
Modified: llvm/trunk/lib/Target/X86/X86FixupBWInsts.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86FixupBWInsts.cpp?rev=272797&r1=272796&r2=272797&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86FixupBWInsts.cpp?rev=272797&r1=272796&r2=272797&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Target/X86/X86FixupBWInsts.cpp (original)<br>
+++ llvm/trunk/lib/Target/X86/X86FixupBWInsts.cpp Wed Jun 15 11:03:06 2016<br>
@@ -95,6 +95,12 @@ class FixupBWInstPass : public MachineFu<br>
/// nullptr.<br>
MachineInstr *tryReplaceCopy(MachineInstr *MI) const;<br>
<br>
+ // Change the MachineInstr \p MI into an eqivalent 32 bit instruction if<br>
+ // possible. Return the replacement instruction if OK, return nullptr<br>
+ // otherwise. Set WasCandidate to true or false depending on whether the<br>
+ // MI was a candidate for this sort of transformation.<br>
+ MachineInstr *tryReplaceInstr(MachineInstr *MI, MachineBasicBlock &MBB,<br>
+ bool &WasCandidate) const;<br>
public:<br>
static char ID;<br>
<br>
@@ -267,6 +273,54 @@ MachineInstr *FixupBWInstPass::tryReplac<br>
return MIB;<br>
}<br>
<br>
+MachineInstr *FixupBWInstPass::tryReplaceInstr(<br>
+ MachineInstr *MI, MachineBasicBlock &MBB,<br>
+ bool &WasCandidate) const {<br>
+ MachineInstr *NewMI = nullptr;<br>
+ WasCandidate = false;<br>
+<br>
+ // See if this is an instruction of the type we are currently looking for.<br>
+ switch (MI->getOpcode()) {<br>
+<br>
+ case X86::MOV8rm:<br>
+ // Only replace 8 bit loads with the zero extending versions if<br>
+ // in an inner most loop and not optimizing for size. This takes<br>
+ // an extra byte to encode, and provides limited performance upside.<br>
+ if (MachineLoop *ML = MLI->getLoopFor(&MBB)) {<br>
+ if (ML->begin() == ML->end() && !OptForSize) {<br>
+ NewMI = tryReplaceLoad(X86::MOVZX32rm8, MI);<br>
+ WasCandidate = true;<br>
+ }<br>
+ }<br>
+ break;<br>
+<br>
+ case X86::MOV16rm:<br>
+ // Always try to replace 16 bit load with 32 bit zero extending.<br>
+ // Code size is the same, and there is sometimes a perf advantage<br>
+ // from eliminating a false dependence on the upper portion of<br>
+ // the register.<br>
+ NewMI = tryReplaceLoad(X86::MOVZX32rm16, MI);<br>
+ WasCandidate = true;<br>
+ break;<br>
+<br>
+ case X86::MOV8rr:<br>
+ case X86::MOV16rr:<br>
+ // Always try to replace 8/16 bit copies with a 32 bit copy.<br>
+ // Code size is either less (16) or equal (8), and there is sometimes a<br>
+ // perf advantage from eliminating a false dependence on the upper portion<br>
+ // of the register.<br>
+ NewMI = tryReplaceCopy(MI);<br>
+ WasCandidate = true;<br>
+ break;<br>
+<br>
+ default:<br>
+ // nothing to do here.<br>
+ break;<br>
+ }<br>
+<br>
+ return NewMI;<br>
+}<br>
+<br>
void FixupBWInstPass::processBasicBlock(MachineFunction &MF,<br>
MachineBasicBlock &MBB) {<br>
<br>
@@ -288,57 +342,61 @@ void FixupBWInstPass::processBasicBlock(<br>
// We run after PEI, so we need to AddPristinesAndCSRs.<br>
LiveRegs.addLiveOuts(MBB);<br>
<br>
+ bool CandidateDidntGetTransformed = false;<br>
+ bool WasCandidate = false;<br>
+<br>
for (auto I = MBB.rbegin(); I != MBB.rend(); ++I) {<br>
- MachineInstr *NewMI = nullptr;<br>
MachineInstr *MI = &*I;<br>
+<br>
+ MachineInstr *NewMI = tryReplaceInstr(MI, MBB, WasCandidate);<br>
<br>
- // See if this is an instruction of the type we are currently looking for.<br>
- switch (MI->getOpcode()) {<br>
-<br>
- case X86::MOV8rm:<br>
- // Only replace 8 bit loads with the zero extending versions if<br>
- // in an inner most loop and not optimizing for size. This takes<br>
- // an extra byte to encode, and provides limited performance upside.<br>
- if (MachineLoop *ML = MLI->getLoopFor(&MBB)) {<br>
- if (ML->begin() == ML->end() && !OptForSize)<br>
- NewMI = tryReplaceLoad(X86::MOVZX32rm8, MI);<br>
- }<br>
- break;<br>
-<br>
- case X86::MOV16rm:<br>
- // Always try to replace 16 bit load with 32 bit zero extending.<br>
- // Code size is the same, and there is sometimes a perf advantage<br>
- // from eliminating a false dependence on the upper portion of<br>
- // the register.<br>
- NewMI = tryReplaceLoad(X86::MOVZX32rm16, MI);<br>
- break;<br>
-<br>
- case X86::MOV8rr:<br>
- case X86::MOV16rr:<br>
- // Always try to replace 8/16 bit copies with a 32 bit copy.<br>
- // Code size is either less (16) or equal (8), and there is sometimes a<br>
- // perf advantage from eliminating a false dependence on the upper portion<br>
- // of the register.<br>
- NewMI = tryReplaceCopy(MI);<br>
- break;<br>
-<br>
- default:<br>
- // nothing to do here.<br>
- break;<br>
- }<br>
-<br>
- if (NewMI)<br>
+ // Add this to replacements if it was a candidate, even if NewMI is<br>
+ // nullptr. We will revisit that in a bit.<br>
+ if (WasCandidate) {<br>
MIReplacements.push_back(std::make_pair(MI, NewMI));<br>
+ if (!NewMI)<br>
+ CandidateDidntGetTransformed = true;<br>
+ }<br>
<br>
// We're done with this instruction, update liveness for the next one.<br>
LiveRegs.stepBackward(*MI);<br>
}<br>
<br>
+ if (CandidateDidntGetTransformed) {<br>
+ // If there was a candidate that didn't get transformed then let's try<br>
+ // doing the register liveness going forward. Sometimes one direction<br>
+ // is overly conservative compared to the other.<br>
+ // FIXME - Register liveness should be investigated further. This really<br>
+ // shouldn't be necessary. See PR28142.<br>
+ LiveRegs.clear();<br>
+ LiveRegs.addLiveIns(MBB);<br>
+<br>
+ auto NextCandidateIter = MIReplacements.begin();<br>
+<br>
+ for (auto I = MBB.begin(); I != MBB.end(); ++I) {<br>
+ MachineInstr *MI = &*I;<br>
+ SmallVector<std::pair<unsigned, const MachineOperand*>, 4> Clobbers;<br>
+ LiveRegs.stepForward(*MI, Clobbers);<br>
+<br>
+ // Only check and create a new instruction if this instruction is<br>
+ // known to be a candidate that didn't get transformed.<br>
+ if (NextCandidateIter->first == MI) {<br>
+ if (NextCandidateIter->second == nullptr) {<br>
+ MachineInstr *NewMI = tryReplaceInstr(MI, MBB, WasCandidate);<br>
+ NextCandidateIter->second = NewMI;<br>
+ }<br>
+ ++NextCandidateIter;<br>
+ }<br>
+ }<br>
+ }<br>
+<br>
while (!MIReplacements.empty()) {<br>
MachineInstr *MI = MIReplacements.back().first;<br>
MachineInstr *NewMI = MIReplacements.back().second;<br>
MIReplacements.pop_back();<br>
- MBB.insert(MI, NewMI);<br>
- MBB.erase(MI);<br>
+ if (NewMI) {<br>
+ MBB.insert(MI, NewMI);<br>
+ MBB.erase(MI);<br>
+ }<br>
}<br>
}<br>
<br>
Added: llvm/trunk/test/CodeGen/X86/fixup-bw-inst-fwlive.mir<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/fixup-bw-inst-fwlive.mir?rev=272797&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/fixup-bw-inst-fwlive.mir?rev=272797&view=auto</a><br>
==============================================================================<br>
--- llvm/trunk/test/CodeGen/X86/fixup-bw-inst-fwlive.mir (added)<br>
+++ llvm/trunk/test/CodeGen/X86/fixup-bw-inst-fwlive.mir Wed Jun 15 11:03:06 2016<br>
@@ -0,0 +1,37 @@<br>
+# RUN: llc -run-pass x86-fixup-bw-insts -mtriple=x86_64-- -o /dev/null %s 2>&1 | FileCheck %s<br>
+<br>
+# Verify that the forwards live-ness checking code in fixup-bw-inst works.<br>
+<br>
+--- |<br>
+ target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"<br>
+<br>
+ define i8 @foo(i8 %p1) {<br>
+ entry:<br>
+ %t1 = or i8 %p1, 0<br>
+ br label %false<br>
+ false:<br>
+ ret i8 %t1<br>
+ }<br>
+<br>
+...<br>
+<br>
+---<br>
+name: foo<br>
+allVRegsAllocated: true<br>
+isSSA: false<br>
+tracksRegLiveness: true<br>
+liveins:<br>
+ - { reg: '%edi' }<br>
+body: |<br>
+ bb.0.entry:<br>
+ liveins: %edi<br>
+ successors: %bb.1.false<br>
+<br>
+ %al = MOV8rr %dil, implicit %edi<br>
+ ; CHECK: %eax = MOV32rr undef %edi, implicit %dil<br>
+<br>
+ bb.1.false:<br>
+ liveins: %al, %ax, %eax, %rax<br>
+ RETQ %al<br>
+<br>
+...<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>