Page MenuHomePhabricator

[GISel]: Add GISelKnownBits analysis
ClosedPublic

Authored by aditya_nandakumar on Aug 3 2019, 9:27 AM.

Details

Summary

This adds a KnownBits analysis pass for GISel. This was done as a pass (compared to static functions) so that we can add other features such as caching queries(within a pass and across passes) in the future. This patch only adds the basic pass boiler plate, and implements a lazy non caching knownbits implementation (ported from SelectionDAG). I've also hooked up the AArch64PreLegalizerCombiner pass to use this - there should be no compile time regression as the analysis is lazy.
I've only the chance to add a couple of tests for now (which check a variety of opcodes), but in the future this should be improved.

Diff Detail

Repository
rL LLVM

Event Timeline

arsenm added inline comments.Aug 3 2019, 4:16 PM
llvm/include/llvm/CodeGen/GlobalISel/GISelKnownBits.h
82

I would expect this to be a pass parameter, not a virtual function

llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
29

Remove Pass to fix PassPass

101

Should probably just be !isVector and allow all pointers

259

I vaguely remember a getSizeInBits being added to MMO? If not it should be

275

Single quotes around single characters

300

I thought these disallowed mismatched sizes

320

Merge consecutive LLVM_DEBUG into one

llvm/unittests/CodeGen/GlobalISel/KnownBitsTest.cpp
26

Gtest defines the order of these the opposite way, so these should be swapped for the correct failure message

52

EXPECT instead of ASSERT?

aditya_nandakumar marked 9 inline comments as done.Aug 4 2019, 4:47 PM
aditya_nandakumar added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/GISelKnownBits.h
82

On second thought, does this need to be configurable at all? I don't see why users would need different depth's for different invocations. Maybe it's best to get rid of this configurability for now and re-add it when necessary.

llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
29

I renamed it to GISelKnownBitsAnalysis.

300

I'm not sure I completely follow. Could you please elaborate?

Updated based on feedback.

arsenm added inline comments.Aug 4 2019, 5:06 PM
llvm/include/llvm/CodeGen/GlobalISel/GISelKnownBits.h
82

The hardcoded depth of 6 always seemed pretty arbitrary. Having to subclass the pass just to change this is too much work, and there’s a non-0 cost to the virtual if it’s configured this way. I don’t particularly care if it’s really configurable though

llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
300

I thought unlike the IR instruction, G_INTTOPTR/G_PTRTOINT mandate the integer size match the point size, but it’s possible I imagined this

aditya_nandakumar marked 2 inline comments as done.Aug 4 2019, 5:17 PM
aditya_nandakumar added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/GISelKnownBits.h
82

Fair enough - I've removed the configurability for now (and the virtual function).

llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
300

I was not aware of such a restriction and from my quick look around the code base, I don't see such a requirement.

dsanders added inline comments.Aug 5 2019, 10:34 AM
llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
300

I thought unlike the IR instruction, G_INTTOPTR/G_PTRTOINT mandate the integer size match the point size, but it’s possible I imagined this

FWIW, I believed this to be the case too but our documentation is rather vague on the subject (we should fix that, Aditya: could you do that as a follow-up?). We have two type-indices but I can't tell if we did that just to allow p0/s64 combinations or if we did it for other reasons too.
The best evidence I can see one way or the other is that LLVM-IR doesn't have this restriction and we haven't said it's difference but that's pretty weak evidence.

Overall I think Aditya's probably right but I expect there's a lot of us who thought this restriction was there.

dsanders accepted this revision.Aug 5 2019, 11:07 AM

LGTM with some nits fixed

llvm/include/llvm/CodeGen/GlobalISel/GISelKnownBits.h
37–39

Do you still need the default-constructor+setMF() set up? I think we could move all the initialization to the constructor and have GISelKnownBitsAnalysis::get() take the MF and allocate/re-use a std::unique_ptr<GISelKnownBits>

What I'm thinking long term when we add caching is that the pass would have an instance of GISelKnownBits per-function and would allocate them lazily as get(MF) is called. Flushing the whole cache is then as simple as freeing the GISelKnownBits and re-constructing it in the next get(MF)

40

Shouldn't this be protected: rather than public:? Will users be calling this themselves?

51–61

These sound like they're potentially protected/private too

63–65

Given that there's no implementation for this yet, let's leave it out until we add caching

80

I believe this should be protected/private too

llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
2–3

Clang-format broke the header formatting here

This revision is now accepted and ready to land.Aug 5 2019, 11:07 AM
paquette added inline comments.Aug 5 2019, 11:13 AM
llvm/unittests/CodeGen/GlobalISel/KnownBitsTest.cpp
32–42

Can we have tests for the other opcodes you're adding support for in this patch?

aditya_nandakumar marked 9 inline comments as done.Aug 5 2019, 12:59 PM
aditya_nandakumar added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/GISelKnownBits.h
37–39

That can certainly work. If I didn't go with the extra unique_ptr, I'd need the default constructor + setMF.
I can update that.

40

This is useful when you're implementing computeKnownBitsForTargetInstr which is in TargetLowering.

51–61

Again - useful for implementing knownBits for target Instrs. For eg, if you have an equivalent instruction to G_FRAME_IDX that's a target PSEUDO, then this allows re-use of existing code.

63–65

Will do.

80

Will change.

llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
2–3

Will fix. Thanks for noticing.

aditya_nandakumar marked 4 inline comments as done.Aug 5 2019, 1:01 PM
aditya_nandakumar added inline comments.
llvm/unittests/CodeGen/GlobalISel/KnownBitsTest.cpp
32–42

Certainly - but I'm strapped for bandwidth right now to exhaustively add test cases for all of them. I ported this test from the IR version of KnownBits unit test.
Can we get this in now and then I can certainly add tests when I have a few cycles?

paquette accepted this revision.Aug 5 2019, 4:37 PM

Given that the patch's test is a port, I think this is fine.

Pushed in 368065.

dsanders added inline comments.Aug 6 2019, 11:36 AM
llvm/include/llvm/CodeGen/GlobalISel/GISelKnownBits.h
89–91

At the moment, if MF varies, the MF inside GISelKnownBits doesn't. You need an instance for each MF.