[GlobalIsel] fix undefined behavior if Action not set.

[igorb]

Summary

It aslo crashing in debug mode.

[kristof.beyls]

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]

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

[kristof.beyls]

In D34978#798827, @igorb wrote:

In 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.

[igorb]

ping

[igorb]

ping

[aditya_nandakumar]

The check seems reasonable to me.

llvm/trunk/include/llvm/CodeGen/GlobalISel/LegalizerInfo.h

public:
   /// type.
   std::tuple<LegalizeAction, unsigned, LLT>
   getAction(const MachineInstr &MI, const MachineRegisterInfo &MRI) const;
 
   /// Iterate the given function (typically something like doubling the width)
   /// on Ty until we find a legal type for this operation.
   Optional<LLT> findLegalizableSize(const InstrAspect &Aspect,
                                     function_ref<LLT(LLT)> NextType) const {
+    if (Aspect.Idx >= Actions[Aspect.Opcode - FirstOp].size())
+      return None;
+
     LegalizeAction Action;
     const TypeMap &Map = Actions[Aspect.Opcode - FirstOp][Aspect.Idx];
     LLT Ty = Aspect.Type;
     do {
       Ty = NextType(Ty);
       auto ActionIt = Map.find(Ty);
       if (ActionIt == Map.end()) {
         auto DefaultIt = DefaultActions.find(Aspect.Opcode);