[PATCH] D34798: [Dominators] Add CFGBuilder testing utility

[Jakub (Kuba) Kuderski via llvm-commits]

>
> Any chance that test cane be ported to this API? Might help demonstrate
> the value, iron out use cases/features/etc. But I guess that test needs
> more than just the CFG - add instructions, etc. Which aren't to be
> supported by this API.


It's easy to port the last test (NonUniqueEdges), the code look like this:
  CFGHolder H("m", "f");
  std::vector<CFGBuilder::Arc> Arcs = {{"bb0", "bb2"}, {"bb0", "bb1"}};
  CFGBuilder B(H.F, Arcs, {});
  BasicBlock *BB0 = B.getOrAddBlock("bb0");
  BasicBlock *BB1 = B.getOrAddBlock("bb1");
  BasicBlock *BB2 = B.getOrAddBlock("bb2");
  auto *Switch = cast<SwitchInst>(BB0->getTerminator());
  Switch->addCase(ConstantInt::get(IntegerType::get(*H.Context, 32), 1),
BB1);

(You can replace the module-building code and the pointer assignments with
it.)
In this particular case, it doesn't give much benefit.

The other test checks dominance of instructions, so it's not that easy to
port :/. The CFGBuilder utility is intended to be used to build and update
(almost) CFG-only IR.

Which parts are not so readable? The code that initializes all the local
> named variables to refer to the BBs & instructions? (YN and BBN)
>

The real improvement comes when you want to execute a series of updates --
I'm working on tests for the incremental DomTree API. As soon as I get that
done, I'll report back with a link to the new revision here -- I don't
intent to submit the CFGBuilder patch imminently :).



On Thu, Jun 29, 2017 at 4:44 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Thu, Jun 29, 2017 at 4:27 PM Jakub Kuderski via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> kuhar added a comment.
>>
>> Thanks for the review, David!
>>
>> > Ah, hmm - I was assuming this was an API for building the CFG data
>> structures. But I see it's an API for building IR that represents
>> particular CFGs (so you can then test the CFG analysis that builds the data
>> structures, etc, I assume?).
>>
>> I tried to clarify that in the class description -- does it help? Perhaps
>> we should think of a better name, any ideas?
>>
>
> I'm not sure it'll be a big deal going forward - once there's a few uses I
> guess most people will use it because they've seen an existing use (if
> they've fixed some CFG analysis feature, they'll go looking for the
> existing tests and find this, see its usage, etc)
>
>
>>
>> > How much more terse is this than the hand-written IR? (I assume that's
>> the motivation/benefit?) How much does that help - perhaps you could show
>> (not in anything that needs to be committed) an example of the textual IR
>> for some of the tests, to demonstrate how much more terse it is? But I
>> guess it's obvious enough perhaps from the tabular update array format that
>> it is quite compact.
>>
>> Well, the thing is that writing the initial IR by hand is not that bad,
>> but then you have to manually iterate over every basic block and assign it
>> to a pointer (or a map of sort). The worst part is actually updating the IR
>> within you test -- then you have to remember to update each terminator,
>> handle edge cases when your successor is default switch case, change
>> terminator to unreachable or return when you remove all the outgoing arcs,
>> etc.
>>
>> As an example, you can look at the ToT DominatorTreeTest.cpp. The IR
>> modified there is really small, yet the update code still manages to pretty
>> lengthy and thus not particularly readable. If you are really interested, I
>> can generate some real-world examples that directly compare the two
>> described approaches.
>>
>
> Any chance that test cane be ported to this API? Might help demonstrate
> the value, iron out use cases/features/etc. But I guess that test needs
> more than just the CFG - add instructions, etc. Which aren't to be
> supported by this API.
>
> Which parts are not so readable? The code that initializes all the local
> named variables to refer to the BBs & instructions? (YN and BBN)
>
>
>>
>> Thanks,
>> ~Kuba
>>
>>
>> https://reviews.llvm.org/D34798
>>
>>
>>
>>


-- 
Jakub Kuderski
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170629/cd4bf565/attachment.html>