[PATCH] D31503: [GlobalISel]: Fix bug where we can report GISelFailure on erased instructions

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Hi Aditya,

If I understand correctly, the problem with the current reporting scheme is the MI we use to print the report may have been deleted, therefore we may access invalid memory.
I would suggest a less invasive change, pretty similar to what we do when we know an iterator is going to be invalidated:
What do you think of printing the MI in a string before calling legalization, then use that string in the error message?

Cheers,
-Quentin



================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerHelper.h:27
 #include "llvm/CodeGen/LowLevelType.h"
+#include "llvm/CodeGen/TargetPassConfig.h"
 
----------------
Use a forward declaration at this point.


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerHelper.h:50
 
-  LegalizerHelper(MachineFunction &MF);
+  LegalizerHelper(MachineFunction &MF, const TargetPassConfig &TPC);
 
----------------
I don't particularly like the fact that the Helper needs to do reporting.
At least, I would expect TPC to be optional.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:82
+      reportGISelFailure(*MF, TPC, MORE, "gisel-legalize",
+                         "unable to legalize instruction", *TmpMI);
       return UnableToLegalize;
----------------
It feels wrong to me to have reporting in here.
My concerns are:
- We conflict reporting and doing the transformation
- We may report failure on product of legalization. Hence, it may actually be difficult to understand where it comes from.


Repository:
  rL LLVM

https://reviews.llvm.org/D31503