Page MenuHomePhabricator

[GlobalIsel] fix undefined behavior if Action not set.
ClosedPublic

Authored by igorb on Jul 4 2017, 4:15 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

igorb created this revision.Jul 4 2017, 4:15 AM

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

igorb added a comment.Jul 4 2017, 5:24 AM

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 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)

Regards,
Igor

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.

The check seems reasonable to me.

This revision was automatically updated to reflect the committed changes.