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.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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? |
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? |
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 |
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. |
llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp | ||
---|---|---|
300 |
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. Overall I think Aditya's probably right but I expect there's a lot of us who thought this restriction was there. |
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 |
llvm/unittests/CodeGen/GlobalISel/KnownBitsTest.cpp | ||
---|---|---|
32–42 | Can we have tests for the other opcodes you're adding support for in this patch? |
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. | |
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. |
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. |
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. |
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)