<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Hi Matt,<div class=""><br class=""></div><div class="">There are two problems with the original commit:</div><div class="">1. The assertion I’ve suggested in review is not where I asked (see inline comment).</div><div class="">2. Before we create the virtual register we need to restrict the register class on the allocatable reg class. I missed that the call to createVirtualRegister was not to the helper function of SDag but the actual call to createVirtualRegister.</div><div class=""><br class=""></div><div class="">The bottom line is that you may still have cases where the allocatable class will not do what you want, but it is required, like I explained in the review. Anyway, by doing that after the constraining failed, you should be in a much better position than previously. If you aren’t in a better position, then you should drop the whole patch.</div><div class=""><br class=""></div><div class="">Cheers,</div><div class="">-Quentin<br class=""><div><blockquote type="cite" class=""><div class="">On Nov 12, 2015, at 1:43 PM, Tom Stellard via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="">Author: tstellar<br class="">Date: Thu Nov 12 15:43:25 2015<br class="">New Revision: 252956<br class=""><br class="">URL: <a href="http://llvm.org/viewvc/llvm-project?rev=252956&view=rev" class="">http://llvm.org/viewvc/llvm-project?rev=252956&view=rev</a><br class="">Log:<br class="">Revert "Remove unnecessary call to getAllocatableRegClass"<br class=""><br class="">This reverts commit r252565.<br class=""><br class="">This also includes the revert of the commit mentioned below in order to<br class="">avoid breaking tests in AMDGPU:<br class=""><br class="">Revert "AMDGPU: Set isAllocatable = 0 on VS_32/VS_64"<br class=""><br class="">This reverts commit r252674.<br class=""><br class="">Modified:<br class=""> llvm/trunk/lib/CodeGen/SelectionDAG/InstrEmitter.cpp<br class=""> llvm/trunk/lib/Target/AMDGPU/SIRegisterInfo.cpp<br class=""> llvm/trunk/lib/Target/AMDGPU/SIRegisterInfo.h<br class=""> llvm/trunk/lib/Target/AMDGPU/SIRegisterInfo.td<br class=""> llvm/trunk/test/CodeGen/AMDGPU/ftrunc.f64.ll<br class=""> llvm/trunk/test/CodeGen/AMDGPU/llvm.round.f64.ll<br class=""><br class="">Modified: llvm/trunk/lib/CodeGen/SelectionDAG/InstrEmitter.cpp<br class="">URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/InstrEmitter.cpp?rev=252956&r1=252955&r2=252956&view=diff" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/InstrEmitter.cpp?rev=252956&r1=252955&r2=252956&view=diff</a><br class="">==============================================================================<br class="">--- llvm/trunk/lib/CodeGen/SelectionDAG/InstrEmitter.cpp (original)<br class="">+++ llvm/trunk/lib/CodeGen/SelectionDAG/InstrEmitter.cpp Thu Nov 12 15:43:25 2015<br class="">@@ -330,15 +330,11 @@ InstrEmitter::AddRegisterOperand(Machine<br class=""> // shrink VReg's register class within reason. For example, if VReg == GR32<br class=""> // and II requires a GR32_NOSP, just constrain VReg to GR32_NOSP.<br class=""> if (II) {<br class="">- const TargetRegisterClass *OpRC = nullptr;<br class="">+ const TargetRegisterClass *DstRC = nullptr;<br class=""> if (IIOpNum < II->getNumOperands())<br class="">- OpRC = TII->getRegClass(*II, IIOpNum, TRI, *MF);<br class="">-<br class="">- if (OpRC && !MRI->constrainRegClass(VReg, OpRC, MinRCSize)) {<br class="">- assert(OpRC->isAllocatable() &&<br class="">- "Constraining an allocatable VReg produced an unallocatable class?");<br class="">-<br class="">- unsigned NewVReg = MRI->createVirtualRegister(OpRC);<br class="">+ DstRC = TRI->getAllocatableClass(TII->getRegClass(*II,IIOpNum,TRI,*MF));<br class="">+ if (DstRC && !MRI->constrainRegClass(VReg, DstRC, MinRCSize)) {<br class="">+ unsigned NewVReg = MRI->createVirtualRegister(DstRC);<br class=""> BuildMI(*MBB, InsertPos, Op.getNode()->getDebugLoc(),<br class=""> TII->get(TargetOpcode::COPY), NewVReg).addReg(VReg);<br class=""> VReg = NewVReg;<br class=""></div></div></blockquote><div><br class=""></div><div>The assertion was supposed to be in the else part, not the if part.</div><div>I.e.,</div><div><b class="">if (DstRC) { // Break this if</b></div><div> if (!MRI->constrainRegClass(VReg, DstRC, MinRCSize)) {</div><div> // This is point #2 I missed.</div><div><b class=""> DstRC = TRI->getAllocatableClass(DstRC); // Refine for allocatable class.</b></div><div><b class=""> assert(DstRC && “Operation constraints cannot be fulfilled for allocation”);</b></div><div> unsigned NewVReg = MRI->createVirtualRegister(DstRC);</div><div> ...</div><div> <b class=""> } else // add this else</b></div><div><blockquote type="cite" class=""></blockquote><b class=""> assert(OpRC->isAllocatable() &&<br class=""> "Constraining an allocatable VReg produced an unallocatable class?”);</b></div><div>}</div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class="">Modified: llvm/trunk/lib/Target/AMDGPU/SIRegisterInfo.cpp<br class="">URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AMDGPU/SIRegisterInfo.cpp?rev=252956&r1=252955&r2=252956&view=diff" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AMDGPU/SIRegisterInfo.cpp?rev=252956&r1=252955&r2=252956&view=diff</a><br class="">==============================================================================<br class="">--- llvm/trunk/lib/Target/AMDGPU/SIRegisterInfo.cpp (original)<br class="">+++ llvm/trunk/lib/Target/AMDGPU/SIRegisterInfo.cpp Thu Nov 12 15:43:25 2015<br class="">@@ -79,6 +79,8 @@ unsigned SIRegisterInfo::getRegPressureS<br class=""> STI.getMaxWavesPerCU());<br class=""> unsigned VGPRLimit = getNumVGPRsAllowed(STI.getMaxWavesPerCU());<br class=""><br class="">+ unsigned VSLimit = SGPRLimit + VGPRLimit;<br class="">+<br class=""> for (regclass_iterator I = regclass_begin(), E = regclass_end();<br class=""> I != E; ++I) {<br class=""> const TargetRegisterClass *RC = *I;<br class="">@@ -86,7 +88,11 @@ unsigned SIRegisterInfo::getRegPressureS<br class=""> unsigned NumSubRegs = std::max((int)RC->getSize() / 4, 1);<br class=""> unsigned Limit;<br class=""><br class="">- if (isSGPRClass(RC)) {<br class="">+ if (isPseudoRegClass(RC)) {<br class="">+ // FIXME: This is a hack. We should never be considering the pressure of<br class="">+ // these since no virtual register should ever have this class.<br class="">+ Limit = VSLimit;<br class="">+ } else if (isSGPRClass(RC)) {<br class=""> Limit = SGPRLimit / NumSubRegs;<br class=""> } else {<br class=""> Limit = VGPRLimit / NumSubRegs;<br class=""><br class="">Modified: llvm/trunk/lib/Target/AMDGPU/SIRegisterInfo.h<br class="">URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AMDGPU/SIRegisterInfo.h?rev=252956&r1=252955&r2=252956&view=diff" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AMDGPU/SIRegisterInfo.h?rev=252956&r1=252955&r2=252956&view=diff</a><br class="">==============================================================================<br class="">--- llvm/trunk/lib/Target/AMDGPU/SIRegisterInfo.h (original)<br class="">+++ llvm/trunk/lib/Target/AMDGPU/SIRegisterInfo.h Thu Nov 12 15:43:25 2015<br class="">@@ -59,6 +59,13 @@ public:<br class=""> /// \returns true if this class contains VGPR registers.<br class=""> bool hasVGPRs(const TargetRegisterClass *RC) const;<br class=""><br class="">+ /// returns true if this is a pseudoregister class combination of VGPRs and<br class="">+ /// SGPRs for operand modeling. FIXME: We should set isAllocatable = 0 on<br class="">+ /// them.<br class="">+ static bool isPseudoRegClass(const TargetRegisterClass *RC) {<br class="">+ return RC == &AMDGPU::VS_32RegClass || RC == &AMDGPU::VS_64RegClass;<br class="">+ }<br class="">+<br class=""> /// \returns A VGPR reg class with the same width as \p SRC<br class=""> const TargetRegisterClass *getEquivalentVGPRClass(<br class=""> const TargetRegisterClass *SRC) const;<br class=""><br class="">Modified: llvm/trunk/lib/Target/AMDGPU/SIRegisterInfo.td<br class="">URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AMDGPU/SIRegisterInfo.td?rev=252956&r1=252955&r2=252956&view=diff" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AMDGPU/SIRegisterInfo.td?rev=252956&r1=252955&r2=252956&view=diff</a><br class="">==============================================================================<br class="">--- llvm/trunk/lib/Target/AMDGPU/SIRegisterInfo.td (original)<br class="">+++ llvm/trunk/lib/Target/AMDGPU/SIRegisterInfo.td Thu Nov 12 15:43:25 2015<br class="">@@ -272,12 +272,9 @@ def SCSrc_32 : RegInlineOperand<SReg_32><br class=""> // VSrc_* Operands with an SGPR, VGPR or a 32-bit immediate<br class=""> //===----------------------------------------------------------------------===//<br class=""><br class="">-def VS_32 : RegisterClass<"AMDGPU", [i32, f32], 32, (add SReg_32, VGPR_32)> {<br class="">- let isAllocatable = 0;<br class="">-}<br class="">+def VS_32 : RegisterClass<"AMDGPU", [i32, f32], 32, (add VGPR_32, SReg_32)>;<br class=""><br class="">-def VS_64 : RegisterClass<"AMDGPU", [i64, f64], 32, (add SReg_64, VReg_64)> {<br class="">- let isAllocatable = 0;<br class="">+def VS_64 : RegisterClass<"AMDGPU", [i64, f64], 32, (add VReg_64, SReg_64)> {<br class=""> let CopyCost = 2;<br class=""> }<br class=""><br class=""><br class="">Modified: llvm/trunk/test/CodeGen/AMDGPU/ftrunc.f64.ll<br class="">URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/AMDGPU/ftrunc.f64.ll?rev=252956&r1=252955&r2=252956&view=diff" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/AMDGPU/ftrunc.f64.ll?rev=252956&r1=252955&r2=252956&view=diff</a><br class="">==============================================================================<br class="">--- llvm/trunk/test/CodeGen/AMDGPU/ftrunc.f64.ll (original)<br class="">+++ llvm/trunk/test/CodeGen/AMDGPU/ftrunc.f64.ll Thu Nov 12 15:43:25 2015<br class="">@@ -27,8 +27,8 @@ define void @v_ftrunc_f64(double addrspa<br class=""> ; SI: s_and_b32 s{{[0-9]+}}, s{{[0-9]+}}, 0x80000000<br class=""> ; SI: s_add_i32 s{{[0-9]+}}, [[SEXP]], 0xfffffc01<br class=""> ; SI: s_lshr_b64<br class="">-; SI-DAG: s_not_b64<br class="">-; SI-DAG: s_and_b64<br class="">+; SI: s_not_b64<br class="">+; SI: s_and_b64<br class=""> ; SI-DAG: cmp_gt_i32<br class=""> ; SI-DAG: cndmask_b32<br class=""> ; SI-DAG: cndmask_b32<br class=""><br class="">Modified: llvm/trunk/test/CodeGen/AMDGPU/llvm.round.f64.ll<br class="">URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/AMDGPU/llvm.round.f64.ll?rev=252956&r1=252955&r2=252956&view=diff" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/AMDGPU/llvm.round.f64.ll?rev=252956&r1=252955&r2=252956&view=diff</a><br class="">==============================================================================<br class="">--- llvm/trunk/test/CodeGen/AMDGPU/llvm.round.f64.ll (original)<br class="">+++ llvm/trunk/test/CodeGen/AMDGPU/llvm.round.f64.ll Thu Nov 12 15:43:25 2015<br class="">@@ -21,7 +21,7 @@ define void @round_f64(double addrspace(<br class=""> ; SI-DAG: v_cmp_eq_i32<br class=""><br class=""> ; SI-DAG: s_mov_b32 [[BFIMASK:s[0-9]+]], 0x7fffffff<br class="">-; SI-DAG: v_cmp_gt_i32<br class="">+; SI-DAG: v_cmp_gt_i32_e32<br class=""> ; SI-DAG: v_bfi_b32 [[COPYSIGN:v[0-9]+]], [[BFIMASK]]<br class=""><br class=""> ; SI: buffer_store_dwordx2<br class=""><br class=""><br class="">_______________________________________________<br class="">llvm-commits mailing list<br class=""><a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a><br class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits<br class=""></div></div></blockquote></div><br class=""></div></body></html>