[PATCH] D16083: [bugpoint] Teach bugpoint to reduce MDNodes

[Adrian Prantl via llvm-commits]

> On Jan 11, 2016, at 1:52 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> +Pete and Adrian, from whom I've heard ideas about bugpointing metadata
> (and debug info specifically) before.
> 
> This is great; I'm glad you're working on this.
> 
>> On 2016-Jan-11, at 13:33, Keno Fischer <kfischer at college.harvard.edu> wrote:
>> 
>> loladiro created this revision.
>> loladiro added reviewers: dexonsmith, tejohnson.
>> loladiro added a subscriber: llvm-commits.
>> Herald added a reviewer: tstellarAMD.
>> Herald added a subscriber: arsenm.
>> 
>> This is essentially pretty simple, it first walks through the the contents of
>> the module starting from instructions/named metadata to find all MDNodes and
>> then does a standard list reduction in order to cut down the list of which
>> metadata nodes to keep. To actually implement the MDNode removal, it creates
>> a temporary Node, inserts it into the VMap, and laters RAUWs that to null.
>> This works great, except that the verifier really doesn't like null values in
>> some MDTuples. Thus, we need some extra logic to also fix up any MDTuples,
>> which would otherwise contain a null.
>> 
>> The one ugly point here is that in order to do the above, we need to allow
>> temporary metadata during cloning. To make this possible, I had to modify
>> CloneModule and CloneFunctionInto, in order to allow passing RemapFlags
>> through. I think this API change is fine, but if there's a different way
>> to accomplish the same thing, I'd be happy to change that. Naturally, I can
>> split that part out into a separate review.
>> 
>> There's also a small fix to pass through AllowTemps recursively in
>> resolveCylces, which was needed to make the above work.
> 
> My intuition is that you're liable to change the crash (rather than
> reduce it) when touching arbitrary metadata like this.  Particularly
> with debug info, this stuff is fairly cross-referential, and a `null`
> reference can have some non-trivial meaning.

Does bugpoint look for any crash or does it look at the backtrace?

> 
> I think we'd have more luck with something that understood the
> structure of the graph, at least at a high-level.
> 
> I've heard Adrian and Pete's ideas on that in the past, maybe they
> can share here?

It is true that blindly hacking at MDNodes can leave us with metadata that satisfies the verifier but is corrupt enough to crash the backend. This means that the reduced testcase will not be very useful once the bug has been fixed. That said, I’m leaning towards that this should be an incentive to improve the verifier. We can still add smarter simplifications that run before this, such as replacing type definitions with forward declarations and the like.


-- adrian
> 
>> http://reviews.llvm.org/D16083
>> 
>> Files:
>> include/llvm/Transforms/Utils/Cloning.h
>> lib/ExecutionEngine/Orc/IndirectionUtils.cpp
>> lib/IR/Metadata.cpp
>> lib/Target/AMDGPU/AMDGPUOpenCLImageTypeLoweringPass.cpp
>> lib/Transforms/Utils/CloneFunction.cpp
>> lib/Transforms/Utils/CloneModule.cpp
>> test/BugPoint/metadata.ll
>> tools/bugpoint-passes/TestPasses.cpp
>> tools/bugpoint/CrashDebugger.cpp
>> tools/bugpoint/ListReducer.h
>> unittests/Transforms/Utils/Cloning.cpp
>> 
>> <D16083.44545.patch>