This is an archive of the discontinued LLVM Phabricator instance.

[IR/GlobalISel] move compare predicates into lib/Support
AbandonedPublic

Authored by t.p.northover on Aug 8 2016, 2:29 PM.

Details

Reviewers
tstellarAMD
Summary

Hi,

I'd like to reuse the IR-level predicate scheme for GlobalISel. As far as I can tell there are 3 possible ways to do this:

  • Use the CmpInst enums directly. This feels close to a layering violation. ISel has to know *something* about IR, but you'd hope to minimize that after the first translation.
  • Copy/paste the definitions into include/CodeGen/GlobalISel somewhere. Long term this isn't viable, though it might be a reasonable short-term hack until we know GlobalISel works.
  • Refactor the predicates out of lib/IR into lib/Support (or elsewhere).

I decided to try and avoid hacks so took the 3rd option, giving us this large diff. I think it's actually a reasonable change on its own and the interfaces are slightly nicer afterwards, though I wouldn't have called it worth the time without GlobalISel's wider intent.

One thing worth thinking about is whether a single Predicate is the best choice. Passes had been (somewhat haphazardly, but not completely without reason) using either CmpInst, ICmpInst or FCmpInst to refer to the enum values.

There's not really any loss of expressiveness with a single Predicate, since the names are still ICMP_*/FCMP_*, but I could alternatively split it into IPredicate and FPredicate which would allow IPredicate::EQ, FPredicate::OEQ and so on.

Opinions?

Diff Detail

Repository
rL LLVM

Event Timeline

t.p.northover retitled this revision from to [IR/GlobalISel] move compare predicates into lib/Support.
t.p.northover updated this object.
t.p.northover set the repository for this revision to rL LLVM.
t.p.northover added a subscriber: llvm-commits.

Just noticed a couple of issues myself:

include/llvm/IR/Instructions.h
1133–1134

Just noticed these are redundant now. I'll remove them.

lib/IR/ConstantFold.cpp
1688

Sorry, these getKind calls are are unnecessary (from an earlier version of the Predicate class). I'll remove them too.

t.p.northover edited edge metadata.

Fixed issues I spotted after upload. Sorry about that!

I don't make sense of hosting these definitions in libSupport.

I think you are creating actually a dependency from GlobalISel to the IR as soon as you want a one-to-one mapping like that and reuse the list.
Just moving it in a separate library just for the purpose of not including llvm/IR/ seems like very artificial to me, and "breaks" the dependency on the surface.

I don't make sense of hosting these definitions in libSupport.

I think you are creating actually a dependency from GlobalISel to the IR as soon as you want a one-to-one mapping like that and reuse the list.
Just moving it in a separate library just for the purpose of not including llvm/IR/ seems like very artificial to me, and "breaks" the dependency on the surface.

FWIW, I agree. If you are relying on a perfect 1:1 matching of the IR predicates to the MI predicates, I don't see much benefit in shifting the header to support. I see some small benefit, but it isn't clear to me that it is worth it...

Sorry not ping, stale comment.