Options.EnableFastISel is true for targets without FastISel implementation

Beginners
1

Hi!

At -O0, the flag TargetMachine.Options.EnableFastISel is true by default even if the target does not implement FastISel. Unfortunately, some code generation passes check this flag.

One example: With SelectionDAG, the failure basic block for a stack protector check contains a call to library function RTLIB::STACKPROTECTOR_CHECK_FAIL. With FastISel, the call to the library function is hardcoded to __stack_chk_fail (or __stack_smash_handler if on OpenBSD).

If I now call setLibcallName() to set a different function name for the stack protector check fail function, then everything is fine except for -O0 - even if the target does not implement FastISel.

IMHO the flag TargetMachine.Options.EnableFastISel should not be true if the target does not implement FastISel. Or do I overlook something? Any idea how to fix this?

Regards,
Kai

2

Do you have a testcase? Do you mean this is broken if you try to explicitly enable fast-isel on a target which doesn’t support it?

3

I expect that Options.EnableFastISel is false by default when the target does not implement FastISel.

Example:

target datalayout = "E-m:e-i1:8:16-i8:8:16-i64:64-f128:64-a:8:16-n32:64"
target triple = "s390x-unknown-linux"

define dso_local signext i32 @bug() #0 {
entry:
  ret i32 0
}

attributes #0 = { noinline nounwind optnone sspreq "frame-pointer"="all" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="z10" }

With llc < bug.ll -stop-after=stack-protector the resulting IR is:

  define dso_local signext i32 @bug() #0 {
  entry:
    %StackGuardSlot = alloca i8*, align 8
    %0 = call i8* @llvm.stackguard()
    call void @llvm.stackprotector(i8* %0, i8** %StackGuardSlot)
    ret i32 0
  }

In the SelectionDAG implementation of the stack protector feature, the check for the stack failure is inserted during lowering.

But with llc < bug.ll -stop-after=stack-protector -O0 the check gets inserted:

  define dso_local signext i32 @bug() #0 {
  entry:
    %StackGuardSlot = alloca i8*, align 8
    %0 = call i8* @llvm.stackguard()
    call void @llvm.stackprotector(i8* %0, i8** %StackGuardSlot)
    %1 = call i8* @llvm.stackguard()
    %2 = load volatile i8*, i8** %StackGuardSlot, align 8
    %3 = icmp eq i8* %1, %2
    br i1 %3, label %SP_return, label %CallStackCheckFailBlk, !prof !0

  SP_return:                                        ; preds = %entry
    ret i32 0

  CallStackCheckFailBlk:                            ; preds = %entry
    call void @__stack_chk_fail()
    unreachable
  }

This output does not change when I additionally specify -enable-selectiondag-sp on the command line.

The condition in StackProtector::InsertStackProtectors() is:

  bool SupportsSelectionDAGSP =
      TLI->useStackGuardXorFP() ||
      (EnableSelectionDAGSP && !TM->Options.EnableFastISel);

TLI->useStackGuardXorFP() is false because the SystemZ target does not use it. EnableSelectionDAGSP is true when I specify -enable-selectiondag-sp on the command line. Since the fastisel code is generated, the flag TM->Options.EnableFastISel must be true in this case. I checked this with adding a llvm::dbgs() statement.

I’m not sure if it is a bug. My expectation is that the EnableFastISel flag (and the O0WantsFastISel flag in the TargetMachine class) only takes a true value if the target has a FastISel implementation.

4

I think you’re right, the logic in TargetPassConfig::addCoreISelPasses looks broken. It’s assuming based on fast-isel should be used at -O0 if the -fast-isel flag is unset, whereas I would assume O0WantsFastIsel would be for the target to control.

However, I think it’s more broken that the StackProtector pass is inspecting the requested selector. On a fallback, IR passes are not re-run. It wouldn’t really matter if the target consistently hit the fallback, but the code here seems to be assuming that will never happen.

5

However, I think it’s more broken that the StackProtector pass is inspecting the requested selector.

Yes, I agree with this.
My first step to fix this would be to add a flag to TargetMachine, indicating if the target implements FastISel or not. This flag can then be used in TargetPassConfig::addCoreISelPasses() when EnableFastISel and O0WantsFastISel. When this is in place, then the uses of EnableFastISel needs to be evaluated if it’s used under the reight assumptions.

6

I’m reluctant to add flags when it is likely that the existing flags aren’t being used in a sensible way. In particular my impression was that O0WantsFastISel was the “target supports FastISel” flag. It would seem odd for a target to support FastISel but not want to use it by default even at O0.

7

This is what I expect, too. The reality from TargetPassConfig is:

  // Enable FastISel with -fast-isel, but allow that to be overridden.
  TM->setO0WantsFastISel(EnableFastISelOption != cl::BOU_FALSE);

Which means that O0WantsFastISel is only false if the user specified -fast-isel=false on the command line. This even holds when the target only implements GlobalISel.

With the additional flag I only want to make sure that the existing flags reflect this properly. E.g.

  TM->setO0WantsFastISel((EnableFastISelOption != cl::BOU_FALSE) &&
                                             TM->supportsFastISel());

Of course, better ideas are welcome.

8

Ah… now that I’ve done the git-blame and resurrected at least some memory of what I was trying to do 6 years ago when I invented it… seems that O0WantsFastISel is a flag on TM only because it needs to be saved/restored somewhere else (in order to handle optnone in a non-O0 scenario). It was in fact never more than a cached copy of the -fast-isel option.

Well! In that case, add a supportsFastISel with my blessing.