[PATCH] D88392: [M68k] (Patch 6/8) IR Tests

[Min-Yih Hsu via Phabricator via llvm-commits]

myhsu marked 2 inline comments as done.
myhsu added inline comments.


================
Comment at: llvm/test/CodeGen/M68k/ASM/Arith/imul-neg.ll:4
+; FIXME: When using SelectionDAGISel, the following cases use
+; `sub` rather than the expected `neg`
+
----------------
RKSimon wrote:
> Please can you raise a bug for these?
Done: https://bugs.llvm.org/show_bug.cgi?id=49244


================
Comment at: llvm/utils/extract-section.py:1
+#!/usr/bin/env python
+from __future__ import print_function
----------------
rengolin wrote:
> RKSimon wrote:
> > myhsu wrote:
> > > jrtc27 wrote:
> > > > myhsu wrote:
> > > > > MaskRay wrote:
> > > > > > If not used, please drop it.
> > > > > > 
> > > > > > `llvm-objcopy --dump-section=.foo=file a.o /dev/null`
> > > > > If you're referring to this script as a whole, it is used extensively in M68k's test suite. 
> > > > > 
> > > > > This script is designed for printing certain section in //textual// format. So I'm afraid your llvm-objcopy command might not be sufficient. As the comment below described, this script is similar to llvm-readobj but the latter only support one output format (i.e. hex with lots of redundant info like ASCii decoding), where in many cases, we want other formats like hex string with different width and textual bits.
> > > > I'm with MaskRay, this script shouldn't exist, use the existing llvm tools.
> > > many of the tests here are checking against binary number, but IIRC the tools LLVM currently have are only able to print out hexadecimal number. The only solution (to get rid of this script) I can think of now is to rewrite all the checks in the tests, but that will be a non-trivial amount of works
> > I have no objections to the extract-section.py as a temporary script but it would most likely have to go before m68k becomes non-experimental. @rengolin ?
> At the very least removed before production, definitely. Though, I'm still undecided if we want to change all the tests or implement this in objdump & friends.
> 
> To @jrtc27's point in another thread, using the existing `update_test_checks` script in LLVM would be a quick way to not have to rely on CHECK lines manually and may be a quick way out of this predicament. But I'm not sure how practical it would be to do this now to the entire set of tests.
> 
> At least now we know that humans have looked at the tests and checked for consistency and correctness. They also express what is expected, rather than what is generated. It's entirely possible that the generations is not perfect (experimental back-end) and we'd be introducing comparisons to things in TODO/FIXME which doesn't help other developers when they break these tests.
> 
> I'd prefer this to be a conscious step on its own, where people can look and make sure we're doing the right thing and expressing the right semantics, rather than a quick fix to merge a big patch series.
> 
> My personal take is that it will be better to land something at least partially good sooner and have more people working directly in LLVM than holding this series for longer and keep the whole m68k community in a suspended state.
> 
> Sure, the code is not perfect. There are stylistic and minor code issues Jessica so diligently spotted. Those can be quickly fixed and rebased, no worries. But bigger changes I'd prefer to keep it for after the first merge.
> 
> Given that it's not yet clear what's the best alternative strategy, I'd create a bug for this, add all the context and opinions expressed here, and mark it as a dependency of the meta-bug for production m68k.
> Given that it's not yet clear what's the best alternative strategy, I'd create a bug for this, add all the context and opinions expressed here

Sounds like a good idea. Here is the bug: https://bugs.llvm.org/show_bug.cgi?id=49245


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

https://reviews.llvm.org/D88392