[PATCH] D34978: [GlobalIsel] fix undefined behavior if Action not set.

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added a comment.

In https://reviews.llvm.org/D34978#798827, @igorb wrote:

> In https://reviews.llvm.org/D34978#798826, @kristof.beyls wrote:
>
> > Hi Igor,
> >
> > Would it be possible to add a regression test for this?
> >  I'm wondering when this fails today. I guess it must be when the target has specified an action for e.g. TypeIdx 0, but not TypeIdx 1?
> >  I'm just trying to understand if this should be an assert (if a target hasn't specified something fully enough) or an if statement.
> >
> > Thanks,
> >
> > Kristof
>
>
> Hi Kristof,
>  I am not sure how to add regression test for this.
>  Today it fails for example on the follow mir
>
>   # RUN: llc -mtriple=x86_64-linux-gnu -O0 -run-pass=legalizer -global-isel %s -o - | FileCheck %s
>     ---
>     name:            test_implicit_def
>     registers:
>     body: |
>     bb.0.entry:
>       liveins:
>  
>       %0:_(s64) = G_IMPLICIT_DEF
>   ...
>
>
> In this case target doesn't set any Action for G_IMPLICIT_DEF.


I find it a bit suspicious that this triggers on G_IMPLICIT_DEF which was added in the past few days, but I don't have time right now to dig in and try and understand the full scope of what's going on.
Given the default action for G_IMPLICIT_DEF is defined as NarrowScalar, my impression is that a target will have to specify at least one size for which "needsLegalizingToDifferentSize" is false, e.g. "Legal".
Otherwise, which size should "NarrowScalar" narrow towards on a G_IMPLICIT_DEF?
That being said, the condition "Aspect.Idx >= Actions[Aspect.Opcode - FirstOp].size()" doesn't seem to be related to the infinite loop of ever more narrowing the type size I explained in the previous sentence?
I'm clearly missing something.
I hope Tim will be able to give you more useful feedback.

> I think the desired behavior of Legalizer is graceful fallback / message and not assert.
> 
>   /bin/llc -O0 -mtriple=x86_64-linux-gnu -mattr=+sse2 -global-isel -run-pass=legalizer  tmp.mir                    
>   LLVM ERROR: unable to legalize instruction: %vreg0<def>(s64) = G_IMPLICIT_DEF; (in function: test_implicit_def)

For what it's worth, I've found it easier to debug issues like this when asserts are being hit rather than when "LLVM ERRORs" are being printed.
It would be good to hear what others think on this aspect.


https://reviews.llvm.org/D34978