[PATCH] D46339: [GlobalISel][Legalizer] LegalizerInfo verifier: Follow Up

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: test/CodeGen/AArch64/GlobalISel/legalize-inttoptr-xfail-1.mir:6
-# This is to demonstrate what kind of bugs we're missing w/o some kind of
-# validation for LegalizerInfo.
 
----------------
Could you put the comment back and describe it in a more useful way?
Basically, call out that we are catching a type mismatch, expecting pointer but got scalar.


================
Comment at: test/CodeGen/AArch64/GlobalISel/legalize-inttoptr-xfail-2.mir:1
-# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
-# RUN: llc -mtriple=aarch64-- -run-pass=legalizer -simplify-mir %s -o - 2>&1 | FileCheck %s
+# RUN: llc -mtriple=aarch64-- -run-pass=legalizer -simplify-mir %s -o - 2>&1
+# XFAIL: *
----------------
rtereshin wrote:
> qcolombet wrote:
> > Should be converted into a `not llc` with a comment explaining why it is incorrect.
> > (Already said it in the other review.)
> What's the policy here to decide between `XFAIL` and `not llc` (or `not opt` etc)?
I don't think there are any.
Personally I tend to avoid XFAIL at all, because you can have crashes and whatnot and it will still happily pass!


================
Comment at: test/CodeGen/AArch64/GlobalISel/legalize-inttoptr-xfail-2.mir:3
 # REQUIRES: asserts
-# XFAIL: *
-
-# This is to demonstrate what kind of bugs we're missing w/o some kind of
-# validation for LegalizerInfo.
 
 # CHECK:      LLVM ERROR: unable to legalize instruction:
----------------
Ditto


Repository:
  rL LLVM

https://reviews.llvm.org/D46339