<div>Thanks, Dale. Though I'm still rebuilding my system after a crash, here a revised patch which addresses some of the issues, with the following notes. This is from an updated tree from Friday, with the 3 sets of llvm tests re-run.</div>
<div> </div>
<div>>There is too much duplication in the different versions of getSingleConstraintMatchWeight. Generic constraints like 'r' should be handled only in the generic version. I think several of the overloaded versions can go away entirely. The approach of substituting ConstraintWeight for numbers is generally OK with this modification.</div>
<div> </div>
<div>Yes, I noticed that, but I put it in anyway to be consistent with the getConstraintType function already there, which I used as a guide. I've removed these in the new patch. The platform implementer can probably figure out when he needs to add one, using another platform as a guide.</div>
<div> </div>
<div>> Should be followed by 'break' (several places). It's disturbing that testing didn't catch this.</div>
<div> </div>
<div>Yes. Sorry, I haven't tested all the platforms yet, only Linux x86. Rather than do this whole thing all at once, I've been trying to do it incrementally. My reasoning is that since multi-alt wouldn't work in Clang anyway, as long as I don't regress on the existing tests, I should be okay, now that I know how to run all three sets of tests. I have a couple of test files I'm working on locally, which I plan to convert to clang and LLVM test files after I get the current patch in.</div>
<div> </div>
<div>Having said that, it just occured to me that perhaps I should be running the gcc tests for multiple platforms, rather than just the default one in the makefile. Do you guys do that? If so, could you give me a list of the triples that I should test?</div>
<div> </div>
<div>To give it a shot, I ran the gcc tests with powerpc-apple-darwin for the build and target in the top-level makefile. Perhaps running the gcc tests targeting platforms other than x86 would have caught this. Here's my results:</div>
<div> </div>
<div>Modified summary:<br># of expected passes 16082<br># of unexpected failures 17933<br># of unexpected successes 29<br># of expected failures 28<br># of unresolved testcases 8666<br># of untested testcases 273<br>
# of unsupported tests 671<br>/home/john/llvm/Release+Asserts/bin/clang version 2.9 (trunk 117191)</div>
<div> </div>
<div>Original summary:<br># of expected passes 16082<br># of unexpected failures 17933<br># of unexpected successes 29<br># of expected failures 28<br># of unresolved testcases 8666<br># of untested testcases 273<br>
# of unsupported tests 671<br>/home/john/llvm_org/Release+Asserts/bin/clang version 2.9 (trunk 117191)<br></div>
<div>Quite a lot fewer passes than in x86, but the results for the modified/original trees are the same.</div>
<div> </div>
<div>
<div>> I need to look at the ParseConstraints bits more, I don't really understand what you're doing there yet.</div>
<div> </div></div>
<div>There are two main parts of it. One part (the beginning and ending chunks of code) was to take some of what was in SelectionDAGBuilder::visitInlineAsm, and some other stuff in SelectionDAGBuilder.cpp such as getCallOperandValEVT, and duplicate it here, to the extent possible without having a DAG. This was necessary because of that one place where parsing the constraints was needed outside of the DAG pass.</div>
<div> </div>
<div>The second part (the code chunk in the middle) does the selection of the constraint alternatives, if there are multiple, using the getMultipleConstraintMatchWeight function to compute the best constraint weight for one alternative, which might have multiple constraint letters. The getSingleConstraintMatchWeight function computes the weight for one constraint character, and is the part to be overidden for different platforms.</div>
<div> </div>
<div>The main idea was that I could inject the support for multiple-alternative constraints without affecting the current logic.</div>
<div> </div>
<div>There are a couple of architectural problems with this in that the parsing could be done multiple times in some circumstances, as opposed to doing it once and saving the results, and there is some duplication of functionality, but changing this would be a much more significant change, and I thought it best to just get it to work reasonably as-is, since the whole inline asm lowering is a pretty complicated set of logic.</div>
<div> </div>
<div>>The bits covered above can go in.</div>
<div> </div>
<div>I think it would be dangerous to try to separate the parts now, so can I just wait until you are comfortable with the whole patch? I'm assuming it won't take much more time. I'm available if you would like me to come over and look at it with you (I'm in Santa Clara).</div>
<div> </div>
<div>Thanks again!</div>
<div> </div>
<div>-John</div>
<div> </div>
<div> </div>
<div class="gmail_quote">On Thu, Oct 21, 2010 at 11:54 AM, Dale Johannesen <span dir="ltr"><<a href="mailto:dalej@apple.com" target="_blank">dalej@apple.com</a>></span> wrote:<br>
<blockquote style="BORDER-LEFT: #ccc 1px solid; MARGIN: 0px 0px 0px 0.8ex; PADDING-LEFT: 1ex" class="gmail_quote">
<div style="WORD-WRAP: break-word">OK, I've gotten to this now. Thanks for your patience.
<div><br></div>
<div>The various changes of std::vector to SmallVector and uses of typedefs are OK.</div>
<div><br></div>
<div>There is too much duplication in the different versions of getSingleConstraintMatchWeight. Generic constraints like 'r' should be handled only in the generic version. I think several of the overloaded versions can go away entirely. The approach of substituting ConstraintWeight for numbers is generally OK with this modification.</div>
<div><br></div>
<div>
<div></div>
<blockquote type="cite">
<div>+ default:</div>
<div>+ weight = TargetLowering::getSingleConstraintMatchWeight(info, constraint);</div></blockquote>
<div><br></div></div>
<div>Should be followed by 'break' (several places). It's disturbing that testing didn't catch this.</div>
<div><br></div>
<div>
<div>
<blockquote type="cite">+ CW_Okay = 0, // Acceptible.</blockquote></div>
<div><br></div></div>
<div>Spelling.</div>
<div><br></div>
<div>I need to look at the ParseConstraints bits more, I don't really understand what you're doing there yet. The bits covered above can go in.</div>
<div>
<div></div>
<div>
<div><br>
<div>
<div>On Oct 15, 2010, at 10:42 AMPDT, John Thompson wrote:</div><br>
<blockquote type="cite">
<div>Thanks for the heads up, Evan.</div>
<div> </div>
<div>I forgot to point out a couple of other changes I made. One was to change some usages of std::vector to SmallVector typedefs on the stuff related to the constraints. The other was that I changed some hard tabs in one file (not from me this time) to spaces, based on the report from a checking program I wrote and am now running on the files I touch.<br>
</div>
<div>-John<br></div>
<div class="gmail_quote">On Fri, Oct 15, 2010 at 10:02 AM, Evan Cheng <span dir="ltr"><<a href="mailto:evan.cheng@apple.com" target="_blank">evan.cheng@apple.com</a>></span> wrote:<br>
<blockquote style="BORDER-LEFT: #ccc 1px solid; MARGIN: 0px 0px 0px 0.8ex; PADDING-LEFT: 1ex" class="gmail_quote">
<div style="WORD-WRAP: break-word">Dale should review this once he gets back on Monday.
<div><br></div>
<div>Evan</div>
<div><br>
<div>
<div>
<div></div>
<div>
<div>On Oct 14, 2010, at 6:41 PM, John Thompson wrote:</div><br></div></div>
<blockquote type="cite">
<div>
<div></div>
<div>
<div>I made a pass through all the platforms, and added support for the weighting of the platform-specific constraints that seemed to have any kind of support, which was not much, based on what was in the existing getConstraintType functions. There probably isn't much point adding weighting for constraints not yet supported, but at least the now framework is there for if and when they are added. Therefore, I'm considering Clang/LLVM mult-alt constraint support done for now, except for some tests I'm getting in shape to check in for both the Clang and LLVM sides. I've run the regression tests, test-suite, and gcc tests on both modified and unmodified trees from this morning on Linux with no differences.</div>
<div> </div>
<div>May I check this in?</div>
<div> </div>
<div>Thanks.</div>
<div> </div>
<div>-John<br clear="all"><br>-- <br>John Thompson<br><a href="mailto:John.Thompson.JTSoftware@gmail.com" target="_blank">John.Thompson.JTSoftware@gmail.com</a><br><br></div></div></div><span><llvmmultalt15.patch></span>_______________________________________________<br>
llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div></blockquote></div><br><br clear="all"><br>-- <br>John Thompson<br><a href="mailto:John.Thompson.JTSoftware@gmail.com" target="_blank">John.Thompson.JTSoftware@gmail.com</a><br><br>_______________________________________________<br>
llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div></div></div></blockquote></div><br><br clear="all"><br>-- <br>John Thompson<br><a href="mailto:John.Thompson.JTSoftware@gmail.com" target="_blank">John.Thompson.JTSoftware@gmail.com</a><br>
<br>