Page MenuHomePhabricator

[GISel]: Attach missing range metadata while translating G_LOADs
ClosedPublic

Authored by aditya_nandakumar on Sun, Jul 21, 5:06 AM.

Details

Summary

Attach range information to MMO while translating Loads.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptSun, Jul 21, 5:06 AM
arsenm added inline comments.Sun, Jul 21, 6:04 AM
llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
892

How does this work if it's split into multiple register? I would assume this needs to be decomposed and only trivially valid if there is one result register

aditya_nandakumar marked an inline comment as done.Sun, Jul 21, 6:14 AM
aditya_nandakumar added inline comments.
llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
892

Good point. I didn't think of that. For now I'll add the ranges only if there is one register.

Attach range information only with one def

arsenm accepted this revision.Sun, Jul 21, 6:30 AM

LGTM. Are you planning on adding a MI computeKnownBits to use this? I will soon have a need for one and don't want to repeat work

This revision is now accepted and ready to land.Sun, Jul 21, 6:30 AM

LGTM. Are you planning on adding a MI computeKnownBits to use this? I will soon have a need for one and don't want to repeat work

Yes - that's how I stumbled on this.
We have an out of tree port of most of DAG's ComputeKnownBits and SimplifyDemandedBits already implemented. It hasn't been upstreamed yet because

  1. There was no need until now.
  2. Writing tests were/are pain and are dependent on combines which upstream didn't have a lot of.

I'd be happy to create a patch for that assuming we decide to go down the route of a direct port of DAG like implementation.

Thanks. r366656

LGTM. Are you planning on adding a MI computeKnownBits to use this? I will soon have a need for one and don't want to repeat work

Yes - that's how I stumbled on this.
We have an out of tree port of most of DAG's ComputeKnownBits and SimplifyDemandedBits already implemented. It hasn't been upstreamed yet because

  1. There was no need until now.
  2. Writing tests were/are pain and are dependent on combines which upstream didn't have a lot of.

    I'd be happy to create a patch for that assuming we decide to go down the route of a direct port of DAG like implementation.

I don't know what could be significantly different

LGTM. Are you planning on adding a MI computeKnownBits to use this? I will soon have a need for one and don't want to repeat work

Yes - that's how I stumbled on this.
We have an out of tree port of most of DAG's ComputeKnownBits and SimplifyDemandedBits already implemented. It hasn't been upstreamed yet because

  1. There was no need until now.
  2. Writing tests were/are pain and are dependent on combines which upstream didn't have a lot of.

    I'd be happy to create a patch for that assuming we decide to go down the route of a direct port of DAG like implementation.

I don't know what could be significantly different

@paquette is going to look at this from a design perspective but we haven't so far had a strong immediate need or the time to do this.

My thoughts are that computeKnownBits in SelectionDAG is a potentially very costly analysis, and we currently have magic recursion depth limits to prevent extremely long compile times. We don't have any evidence to support an alternative yet, but it would be nice to prototype different approaches for the GlobalISel equivalent. Maybe add caching support for the start, and/or implement it using a bottom up approach.

LGTM. Are you planning on adding a MI computeKnownBits to use this? I will soon have a need for one and don't want to repeat work

Yes - that's how I stumbled on this.
We have an out of tree port of most of DAG's ComputeKnownBits and SimplifyDemandedBits already implemented. It hasn't been upstreamed yet because

  1. There was no need until now.
  2. Writing tests were/are pain and are dependent on combines which upstream didn't have a lot of.

    I'd be happy to create a patch for that assuming we decide to go down the route of a direct port of DAG like implementation.

I don't know what could be significantly different

@paquette is going to look at this from a design perspective but we haven't so far had a strong immediate need or the time to do this.

My thoughts are that computeKnownBits in SelectionDAG is a potentially very costly analysis, and we currently have magic recursion depth limits to prevent extremely long compile times. We don't have any evidence to support an alternative yet, but it would be nice to prototype different approaches for the GlobalISel equivalent. Maybe add caching support for the start, and/or implement it using a bottom up approach.

It should be quite trivial to add caching support to compute known bits while operating on demand similar to DAG (which is pretty much bottom up).

For now, I would be happy with the interface that handles the most trivial cases so I can use it in code I’m porting so I don’t have to think about it later. Having it functionally complete is less important