[PATCH] D154348: Ignore modified attribute list if it yields invalid IR

[Brian Gesiak via Phabricator via llvm-commits]

modocache requested changes to this revision.
modocache added a comment.
This revision now requires changes to proceed.

Looks great!! Thank you for also updating the other call sites. I had a few additional comments, but once those are taken care of I can accept. (When I do, will you need me to merge this commit on your behalf?)



================
Comment at: llvm/tools/bugpoint/CrashDebugger.cpp:73
 
+static bool isValidModule (std::unique_ptr<Module> &M, bool exitOnFailure=true) {
+  bool isNotValid = llvm::verifyModule(*(M.get()), &llvm::errs());
----------------
Please run `clang-format` on this patch; I can tell from some of the spacing here that this is not typical formatting. I believe `clang-format` should format this as `static bool isValidModule(std::unique_ptr<Module> &M, bool exitOnFailure = true) {`. There may be other formatting changes that would be applied to other parts of your patch as well.

Also, there's a mix of capitalization conventions here. I believe the Coding Standards here -- https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly -- would have you name this `ExitOnFailure`, with a capital 'E'.


================
Comment at: llvm/tools/bugpoint/CrashDebugger.cpp:74
+static bool isValidModule (std::unique_ptr<Module> &M, bool exitOnFailure=true) {
+  bool isNotValid = llvm::verifyModule(*(M.get()), &llvm::errs());
+  if (isNotValid) {
----------------
I believe you don't need the parentheses here.


================
Comment at: llvm/tools/bugpoint/CrashDebugger.cpp:75
+  bool isNotValid = llvm::verifyModule(*(M.get()), &llvm::errs());
+  if (isNotValid) {
+    if (exitOnFailure) {
----------------
I think this would be a little simpler if you checked the inverse:

```
if (!llvm::verifyModule(*M.get(), &llvm::errs()))
  return true;

if (ExitOnFailure) {
  llvm::errs() << "verify failed!\n";
  exit(1);
}
return false;
```

This removes a lot of nesting in your `if` statements, and I think looks nicer.


================
Comment at: llvm/tools/bugpoint/CrashDebugger.cpp:794
 
-  // Verify that this is still valid.
-  legacy::PassManager Passes;
-  Passes.add(createVerifierPass(/*FatalErrors=*/false));
-  Passes.run(*M);
+  // Verify we didn't break anything
+  isValidModule(M);
----------------
No need to update this comment, IMHO.


================
Comment at: llvm/tools/bugpoint/CrashDebugger.cpp:795
+  // Verify we didn't break anything
+  isValidModule(M);
 
----------------
I believe this should use `ExitOnFailure = false`, right?


================
Comment at: llvm/tools/bugpoint/CrashDebugger.cpp:864
 
-  // Verify that this is still valid.
-  legacy::PassManager Passes;
-  Passes.add(createVerifierPass(/*FatalErrors=*/false));
-  Passes.run(*M);
+  // Verify we didn't break anything
+  isValidModule(M);
----------------
Same as above; I don't believe this comment should change. Same goes for the 2 spots below in this patch.


================
Comment at: llvm/tools/bugpoint/CrashDebugger.cpp:865
+  // Verify we didn't break anything
+  isValidModule(M);
 
----------------
Same as above; I believe the previous behavior was to not exit. Same goes for the 2 spots below in this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154348