[llvm] [GlobalISel] Combine G_ZEXT of undef -> 0 (PR #113764)

[Amara Emerson via llvm-commits]

Thorsten =?utf-8?q?Schütt?= <schuett at gmail.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/113764 at github.com>


================
@@ -2916,8 +2916,11 @@ void CombinerHelper::replaceInstWithFConstant(MachineInstr &MI, double C) {
 
 void CombinerHelper::replaceInstWithConstant(MachineInstr &MI, int64_t C) {
   assert(MI.getNumDefs() == 1 && "Expected only one def?");
-  Builder.buildConstant(MI.getOperand(0), C);
-  MI.eraseFromParent();
+  LLT DstTy = MRI.getType(MI.getOperand(0).getReg());
+  if (isConstantLegalOrBeforeLegalizer(DstTy)) {
----------------
aemerson wrote:

> Furthermore, I prefer the legality check to be as close as possible to the build to prevent bugs.

I don't see how one can hold this preference and yet at the same time be completely ok with the fact that the place which is named specifically for the job this is trying to do, match patterns, is not the place where the code lives. Instead the check is being added to a function who's sole purpose is to execute a transformation which has already been checked for legality in other senses, such as correctness and code quality heuristics, but now can fall over at the last second because of a completely random check that's only there because someone thought it would prevent bugs.

Even if you thought the matcher was a sub-optimal place to put the check due to ease-of-bugs issues, the right approach to solving the problem would surely be to think of another design, perhaps extend the tablegen-based combiner infrastructure to allow expressing these predicates in more ergonomic ways? Or is this your approach because it's simply the quickest way to do it?

https://github.com/llvm/llvm-project/pull/113764