[llvm] r268452 - PM: Port LoopRotation to the new loop pass manager

[Chandler Carruth via llvm-commits]

On Tue, May 10, 2016 at 11:39 PM Xinliang David Li <xinliangli at gmail.com>
wrote:

>
>
> On Tue, May 10, 2016 at 10:08 PM, Justin Bogner <mail at justinbogner.com>
> wrote:
>
>> Xinliang David Li via llvm-commits <llvm-commits at lists.llvm.org> writes:
>> > On Tue, May 10, 2016 at 4:43 PM, Chandler Carruth via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>> > On Tue, May 10, 2016 at 5:07 PM Sean Silva via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>> >>> Using a function called "getCachedResult" for this purpose seems a bit
>> >>> off in the first place. I think it would make sense to call it
>> >>> `getRequiredResult` or something like that so that it expresses the
>> >>> semantic intent ("as a loop pass, I can't force this to be computed,
>> >>> but I require it so it better be there").
>> >>
>> >> I think "cached" is still really important -- it signifies that unlike
>> >> other query paths it won't go and do work all of a sudden. =]
>> >>
>> >> That said, I agree that the pattern of requiring a cached result from
>> an
>> >> outer pass manager is very common and we should streamline this.
>> >>
>> >> The reason I hadn't actually done it is that I'm actually *really* torn
>> >> about whether it would be better to have passes just skip their logic
>> or
>> >> fail hard. I almost *every* case I prefer the assert or fatal error...
>> >> except when it comes down to bisecting a failing pass or similar
>> >> techniques.
>> >
>> > How about this:   getCachedResults can either return null or fail hard
>> > depending on an internal option.  The client code shares the same
>> boiler plate
>> > -- skips/bails when null is returned from getCachedResult. In normal
>> build,
>> > you would get assertion/fail, but bitsecting tools can disable the
>> assert.
>>
>> That doesn't quite work - there are certainly cases where we use
>> getCachedResults because it really is optional.
>>
>
> getCachedResults can have an additional parameter indicating whether it is
> truly required or optional. For optional request, never assert/fail.
>
>>
>> We might be able to combine this idea with adding a second API that
>> says "I really need this result from the cache and I can't progress
>> without it".
>>
>
> One interface may suffice -- the additional parameter can default to false
> (i.e., by default it is not optional).
>

I really don't like using parameters this way. If we want a different
semantic model it should have a different API name IMO.

But I don't think we need a switch controlling this and having one doesn't
actually help any.

For pass pipeline random exploration and such, you really need adding the
loop pass without the analysis is a no-op as opposed to an error (whether
assert or otherwise).

But maybe with a good API for handling the fatal errors and by not using
assert at all but report_fatal_error we'll be able to do more fuzzing-style
exploration by detecting the failures. It doesn't seem like a fantastic
solution, but not the worst. And I don't think we're painting ourselves
into a corner here so I'm not too worried about it.


>
> David
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160511/372e6e7b/attachment.html>