[X86ISelLowering] permit BlockAddressSDNode "i" constraints for PIC

[nickdesaulniers]

Summary

When building 32b x86 code as PIC, the existing handling of "i"
constraints is conservative since generally we have to go through the
GOT to find references to functions.

But generally, BlockAddresses from C code refer to the Function in the
current TU. Permit BlockAddresses to be used with the "i" constraint
for those cases.

I regressed this in
commit 4edb9983cb8c ("[SelectionDAG] treat X constrained labels as i for asm")

Fixes: https://github.com/llvm/llvm-project/issues/53868

[nickdesaulniers]

• manually format like a peasant

llvm/lib/Target/X86/X86ISelLowering.cpp

54796

interesting, the code formatter didn't run on this file. Probably because it's too big.

[nickdesaulniers]

Looks like the Subtarget.isPICStyleGOT() || Subtarget.isPICStyleStubPIC() check was added in

commit 5ad5226c58c8 ("Disallow matching "i" constraint to symbol addresses when address requires a register or secondary load to compute (most PIC modes). This improves "g" constraint handling. 8015842.")

[efriedma]

I don't think you need to check that the blockaddress refers to the current function. A blockaddress always refers to the version of the function in the current file (similar to the way an alias works). So the address is always going to be valid; you never have to go through a GOT or anything like that.

I think there are other issues with this code; it isn't handling all the patterns that it should. See immediate handling in TargetLowering::LowerAsmOperandForConstraint.

[nickdesaulniers]

• don't check blockaddress Function, as per @efriedma. Merge tests

[nickdesaulniers]

In D119905#3327353, @efriedma wrote:

I think there are other issues with this code; it isn't handling all the patterns that it should. See immediate handling in TargetLowering::LowerAsmOperandForConstraint.

Luckily, the target specific overrides defer to the base classes method (by explicitly calling TargetLowering::LowerAsmOperandForConstraint from X86TargetLowering::LowerAsmOperandForConstraint. It's below the fold of what phab shows, but it's just a few lines down from my hunk if you expand the view in phab.

[nickdesaulniers]

In D119905#3327353, @efriedma wrote:

A blockaddress always refers to the version of the function in the current file (similar to the way an alias works).

I'm not sure that holds under LTO. Otherwise I don't think we'd have bugs like: https://github.com/llvm/llvm-project/issues/52787.

[efriedma]

In D119905#3327585, @nickdesaulniers wrote:

In D119905#3327353, @efriedma wrote:

A blockaddress always refers to the version of the function in the current file (similar to the way an alias works).

I'm not sure that holds under LTO. Otherwise I don't think we'd have bugs like: https://github.com/llvm/llvm-project/issues/52787.

It has to hold, or else everything just explodes. blockaddresses aren't in the public symbol table of an object file.

This does make LTO a bit more complicated, but not impossible. (e.g. ThinLTO can't import functions from other bitcode files if they refer to a blockaddress.)

[efriedma]

LGTM

[nickdesaulniers]

(I plan on requesting this be backported to clang-14 to fix https://github.com/llvm/llvm-project/issues/53868; I'm going to be on vacation next week so I'm going to land this + request backport ASAP).

Thanks for the review + feedback.

[MaskRay]

Consider placing the test in an existing ${0:l} test, e.g. asm-block-labels.ll

[nickdesaulniers]

• move unit tests into existing llvm/test/CodeGen/X86/inline-asm-pic.ll

[nickdesaulniers]

In D119905#3327864, @MaskRay wrote:

Consider placing the test in an existing ${0:l} test, e.g. asm-block-labels.ll

llvm/test/CodeGen/X86/inline-asm-pic.ll seemed more appropriate (-m32 && -fPIC).

Thanks for the review!

llvm/lib/Target/X86/X86ISelLowering.cpp

if (ConstantSDNode *CST = dyn_cast<ConstantSDNode>(Op)) {
       int64_t ExtVal = ExtOpc == ISD::ZERO_EXTEND ? CST->getZExtValue()
                                                   : CST->getSExtValue();
       Result = DAG.getTargetConstant(ExtVal, SDLoc(Op), MVT::i64);
       break;
     }
 
     // In any sort of PIC mode addresses need to be computed at runtime by
     // adding in a register or some sort of table lookup.  These can't
-    // be used as immediates.
-    if (Subtarget.isPICStyleGOT() || Subtarget.isPICStyleStubPIC())
+    // be used as immediates. BlockAddresses are fine though.
+    if ((Subtarget.isPICStyleGOT() || Subtarget.isPICStyleStubPIC()) &&
+        !isa<BlockAddressSDNode>(Op))
       return;
 
     // If we are in non-pic codegen mode, we allow the address of a global (with
     // an optional displacement) to be used with 'i'.
     if (auto *GA = dyn_cast<GlobalAddressSDNode>(Op))
       // If we require an extra load to get this address, as in PIC mode, we
       // can't accept it.
       if (isGlobalStubReference(
               Subtarget.classifyGlobalReference(GA->getGlobal())))
         return;

llvm/test/CodeGen/X86/inline-asm-pic.ll

 ; CHECK-NEXT:    ## InlineAsm Start
 ; CHECK-NEXT:    movl %eax, %gs:152
 ; CHECK-NEXT:    ## InlineAsm End
 ; CHECK-NEXT:    retl
 entry:
   tail call void asm "mov $1,%gs:$0", "=*m,ri,~{dirflag},~{fpsr},~{flags}"(i8** elementtype(i8*) inttoptr (i32 152 to i8**), i8* bitcast (i8** @main_q to i8*)) nounwind
   ret void
 }
+
+; The intent of this test is to ensure that we handle blockaddress' correctly
+; with "i" constraints for -m32 -fPIC.
+
+define void @x() {
+; CHECK-LABEL: x:
+; CHECK:       ## %bb.0:
+; CHECK-NEXT:    ## InlineAsm Start
+; CHECK-NEXT:    ## Ltmp0
+; CHECK-EMPTY:
+; CHECK-NEXT:    ## InlineAsm End
+; CHECK-NEXT:  ## %bb.2: ## %return
+; CHECK-NEXT:    retl
+; CHECK-NEXT:  Ltmp0: ## Block address taken
+; CHECK-NEXT:  LBB1_1: ## %overflow
+; CHECK-NEXT:    retl
+  callbr void asm "#  ${0:l}\0A", "i"(i8* blockaddress(@x, %overflow))
+          to label %return [label %overflow]
+
+overflow:
+  br label %return
+
+return:
+  ret void
+}
+
+; Test unusual case of blockaddress from @x in @y's asm.
+define void @y() {
+; CHECK-LABEL: y:
+; CHECK:       ## %bb.0:
+; CHECK-NEXT:    ## InlineAsm Start
+; CHECK-NEXT:    ## Ltmp0
+; CHECK-EMPTY:
+; CHECK-NEXT:    ## InlineAsm End
+; CHECK-NEXT:    retl
+  call void asm "# ${0:l}\0A", "i"(i8* blockaddress(@x, %overflow))
+  ret void
+}