[PATCH] D63672: Added Delta IR Reduction Tool

[Diego Treviño via Phabricator via llvm-commits]

diegotf marked 2 inline comments as done.
diegotf added a comment.

In D63672#1619572 <https://reviews.llvm.org/D63672#1619572>, @dblaikie wrote:

> Also, I'm not able to run these locally - because I think I'm not using the system shell. Do you know anytihng about how I tell lit not to use its own shell, but to use the system one so it'll run these tests? (I can remove the REQUIRES line from the test and they run OK I think - but would be best to reproduce it more accurately) &


That's weird, the test should work with lit's shell just fine. And as far as I know the workaround that you mentioned is the only solution I know.

> have you been able to reproduce the failures various folks were reporting when this was previously committed?

Locally, no. So I'm going to add a bit more logging to see why it broke the buildbots



================
Comment at: llvm/tools/llvm-reduce/deltas/Delta.h:33
+/// Writes IR code to the given Filepath
+inline bool writeProgramToFile(SmallString<128> Filepath, int FD,
+                               const Module &M) {
----------------
dblaikie wrote:
> diegotf wrote:
> > dblaikie wrote:
> > > This seems like a fairly general function name to have in the llvm namespace in a header - yeah, all these functions look like they aren't part of the API here, but helper functions for the template below. So hiding them in some way would be nice. We don't have many cases of "impl"/"detail" namespaces in LLVM - maybe a trivial base class? (I'm not a huge fan of using classes to group functions, so this doesn't sound super satisfying to me - I'm open to ideas/suggestions)
> > Agreed, I will just move them out of the llvm namespace for the moment, since I'm still on the fence about this class' design
> I'm still a bit concerned the != is inconsistent with op< (eg: there could be two items for which !(A < B) and !(B < A) (ie: the items are equivalent, as far as the < ordering is concerned) but A != B). 
> 
> Is the inequality needed/does it need to be narrowly defined as it is? If so, I'd suggest a proper ordering of op<, then.
> 
> Maybe it's simple enough that you could define op< as "std::tie(C1.begin, C1.end) < std::tie(C2.begin, C2.end)"
Yesterday I was debugging a problem caused precisely by this inconsistency.

I didn't know std::tie even existed, I'll change op< to your suggestion :)


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63672/new/

https://reviews.llvm.org/D63672