[PATCH] Fix illegal DAG produced by SelectionDAG::getConstant() for v2i64 type

[Daniel Sanders]

In that case it sounds like I should be committing without needing a further LGTM. Thanks.

The reason I wasn't sure was that in this case, making the requested change ended up requiring more than just the simple change that was requested. Adding the 'virtual' keyword revealed that some of the new tests were incorrectly passing in the original patch. In fixing this bug, I ended up deleting the call to the function I was asked to change (and then deleted the function itself because it was now unused and the original patch added it). The patch is still largely the same in all important respects. The only code-path that changed handles a MIPS-specific case.

From: David Blaikie [mailto:dblaikie at gmail.com]
Sent: 13 November 2013 17:19
To: reviews+D1973+public+5bd1dfc967a43b3a at llvm-reviews.chandlerc.com
Cc: Benjamin Kramer; Daniel Sanders; llvm-commits at cs.uiuc.edu
Subject: Re: [PATCH] Fix illegal DAG produced by SelectionDAG::getConstant() for v2i64 type



On Wed, Nov 13, 2013 at 4:03 AM, Daniel Sanders <daniel.sanders at imgtec.com<mailto:daniel.sanders at imgtec.com>> wrote:

  Can I check a matter of policy?

  Is it ok to commit the version of the patch you LGTM'd and then fix the MIPS-specific false passes (caused by the missing 'virtual' keyword) under the 'obvious change' rule? or should I wait for an explicit LGTM on the updated patch?

Generally if someone says "LGTM as long as you do blah" you're entrusted to make those changes in your current patch and commit in one go without further review. If for some reason you got the requested changes wrong, they'll generally be addressed in post-commit review (which, if the wrongness was sufficiently severe, could include reverting, but that's usually uncommon/unlikely as such extra changes are usually simple/mechanical in nature)


  I don't mind either way. I just want to double-check what I should be doing for this patch.

  Thanks

http://llvm-reviews.chandlerc.com/D1973

BRANCH
  msa/bugfix-getconstant2

ARCANIST PROJECT
  llvm
_______________________________________________
llvm-commits mailing list
llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131114/b81d2b8f/attachment.html>