Emit diagnostic if an inline asm constraint requires an immediate

[void]

Summary

An inline asm call can result in an immediate after inlining. Therefore emit a
diagnostic here if constraint requires an immediate but one isn't supplied.

[void]

Fix testcase to get the correct error message.

[void]

Add testcase.

[mgorny]

I can confirm that it fixes our issue but I don't feel confident reviewing the code itself. Copying reviewers from the original diff.

[joerg]

It does not fix the issues on our side, but pushes them to a different place. It is still an improvement, but the problem is not solved yet.

[void]

In D60942#1477583, @joerg wrote:

It does not fix the issues on our side, but pushes them to a different place. It is still an improvement, but the problem is not solved yet.

Joerg, What's the error again? (I forgot, sorry.)

[joerg]

On the frontend side:

• __builtin_constant_p is not expanded correctly in EvalConstant.cpp when used as part of codegen for -O0. This means that trivially unreachable branches are emitted (e.g. with a BR with a literal false as condition),
• The constant evaluator has a general problem with overly pessimistic analysis when it comes to possible side effects. Still have to discuss the correct handling with Richard for this.

On the backend side:

• The is_constant intrinsic is lowered too late. The prepare-isel pass it is currently part of is run after the last SimplifyCFG pass, so the now-dead basic blocks are never pruned and therefore the asm statements survive.

All that said, with the discussed generalization from literal 'n' to 'expects immediate', I think this code solves the part of the problem it is meant to solve.

[void]

In D60942#1500730, @joerg wrote:

On the frontend side:

• __builtin_constant_p is not expanded correctly in EvalConstant.cpp when used as part of codegen for -O0. This means that trivially unreachable branches are emitted (e.g. with a BR with a literal false as condition),
• The constant evaluator has a general problem with overly pessimistic analysis when it comes to possible side effects. Still have to discuss the correct handling with Richard for this.

GCC has this to say about __builtin_constant_p's behavior:

A return of 0 does not indicate that the value is not a constant, but merely that GCC cannot prove it is a constant with the specified value of the -O option.

When I was modifying our implementation recently, I made it so that if there are no optimizations then we evaluate to 0 in the front-end before code-gen. It might be that those blocks are being emitted, but they should have something like br i1 false, label %true_bb, label %false_bb or equivalent.

On the backend side:

• The is_constant intrinsic is lowered too late. The prepare-isel pass it is currently part of is run after the last SimplifyCFG pass, so the now-dead basic blocks are never pruned and therefore the asm statements survive.

Is this at -O0 or all optimization levels?

All that said, with the discussed generalization from literal 'n' to 'expects immediate', I think this code solves the part of the problem it is meant to solve.

W00T!

[joerg]

In D60942#1500742, @void wrote:

In D60942#1500730, @joerg wrote:

On the frontend side:

• __builtin_constant_p is not expanded correctly in EvalConstant.cpp when used as part of codegen for -O0. This means that trivially unreachable branches are emitted (e.g. with a BR with a literal false as condition),
• The constant evaluator has a general problem with overly pessimistic analysis when it comes to possible side effects. Still have to discuss the correct handling with Richard for this.

GCC has this to say about __builtin_constant_p's behavior:

A return of 0 does not indicate that the value is not a constant, but merely that GCC cannot prove it is a constant with the specified value of the -O option.

When I was modifying our implementation recently, I made it so that if there are no optimizations then we evaluate to 0 in the front-end before code-gen. It might be that those blocks are being emitted, but they should have something like br i1 false, label %true_bb, label %false_bb or equivalent.

Yes, that's correct. The problem is that with -O0, we never run SimplifyCFG, so those conditional branches make it straight to the backend. As discussed with Richard on IRC, the core issue is that for -O0, the IR generation for __builtin_constant_p is different from the constant folding as part of AST/EvalConstant.cpp. The latter is used by EmitIfStmt and friends to decide whether the BBs should be created in first place and currently fails evaluation for bcp.

On the backend side:

• The is_constant intrinsic is lowered too late. The prepare-isel pass it is currently part of is run after the last SimplifyCFG pass, so the now-dead basic blocks are never pruned and therefore the asm statements survive.

Is this at -O0 or all optimization levels?

With -O0, the intrinsic is never generated and no SimplifyCFG is run either (see above).

[void]

In D60942#1500749, @joerg wrote:

In D60942#1500742, @void wrote:

In D60942#1500730, @joerg wrote:

On the frontend side:

• __builtin_constant_p is not expanded correctly in EvalConstant.cpp when used as part of codegen for -O0. This means that trivially unreachable branches are emitted (e.g. with a BR with a literal false as condition),
• The constant evaluator has a general problem with overly pessimistic analysis when it comes to possible side effects. Still have to discuss the correct handling with Richard for this.

GCC has this to say about __builtin_constant_p's behavior:

A return of 0 does not indicate that the value is not a constant, but merely that GCC cannot prove it is a constant with the specified value of the -O option.

When I was modifying our implementation recently, I made it so that if there are no optimizations then we evaluate to 0 in the front-end before code-gen. It might be that those blocks are being emitted, but they should have something like br i1 false, label %true_bb, label %false_bb or equivalent.

Yes, that's correct. The problem is that with -O0, we never run SimplifyCFG, so those conditional branches make it straight to the backend. As discussed with Richard on IRC, the core issue is that for -O0, the IR generation for __builtin_constant_p is different from the constant folding as part of AST/EvalConstant.cpp. The latter is used by EmitIfStmt and friends to decide whether the BBs should be created in first place and currently fails evaluation for bcp.

On the backend side:

• The is_constant intrinsic is lowered too late. The prepare-isel pass it is currently part of is run after the last SimplifyCFG pass, so the now-dead basic blocks are never pruned and therefore the asm statements survive.

Is this at -O0 or all optimization levels?

With -O0, the intrinsic is never generated and no SimplifyCFG is run either (see above).

Ah! I understand now. I'll see if there's something I can do to help with this. Could you send me some test cases so that I can test things?

[mgorny]

Is there anything blocking this part from being committed?

[joerg]

The patch still needs to be generalized to handle all ICE constraints, but it is not blocked by the review for handling the is.constant intrinsic.

[void]

Expand the diagnostic to support all constraints that require immediates.

[void]

I added a new constraint type C_Immdediate which indicates that a constraint requires an immediate value.

Please take a look.

[void]

Friendly ping.

[void]

Updating.

[void]

Revert the prvious update

[void]

Revert accidentally added files.

[joerg]

lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

439 ↗

(On Diff #210713)

This is an unrelated chunk. Can you drop it please? I like the idea, but it would be nicer if it didn't need to create an intermediate label at all.

[void]

Remove file that sneaked in.

lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

439 ↗

(On Diff #210713)

Ah! Yes, this sneaked in there...

[joerg]

With the exception of the small improvements outlined, this looks good to me.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

7851

Unrelated, but shouldn't Flags be filtered first? E.g.

Flags &= ~(InlineAsm::Extra_MayLoad | InlineAsm::Extra_MayStore);

There seems to be one test case (PowerPC/xray-ret-is-terminate.ll) that depends on this somewhat.

Adding C_Immediate here seems to be wrong though, since immediates are never loads or stores. As such, shall we just drop this chunk and bring up the Flags update separate?

7941

like the "n" constraint

lib/Target/X86/X86ISelLowering.cpp

44780 ↗

(On Diff #212029)

'L' means 0xFF or 0xFFFF, that should be C_Immediate.

44781 ↗

(On Diff #212029)

'M' means 0..3 as shifts for LEA, should be C_Immediate as well.

[void]

Mark some constraints which require immediates as needing immediates.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

7851

I wanted to retain the original behavior, but yeah it seems like C_Immediate isn't really worth checking here.

As for the Flags, wouldn't clearing the flags remove previous iterations which set them? This is being called in a loop over all asm contraints...

[joerg]

LGTM otherwise.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

7941

Typo still.

[void]

"like" :: "line" -- "potAYto" :: "potAHto"

:-)

[void]

Emit an error for an out-of-range immediate.

[void]

Update testing. This adds support for RISC-V inline asm immediate constraints
so that we emit the same errors that the front-end would.

[void]

I've updated this patch to better coincide with D60943. It adds support for RISC-V inline asm immediate constraints (to make the errors match those of the front-end) and a few more tests. Please take another look, since these are non-trivial changes.

Thanks!

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

public:
   }
 
   void update(const TargetLowering::AsmOperandInfo &OpInfo) {
     // Ideally, we would only check against memory constraints.  However, the
     // meaning of an Other constraint can be target-specific and we can't easily
     // reason about it.  Therefore, be conservative and set MayLoad/MayStore
     // for Other constraints as well.
     if (OpInfo.ConstraintType == TargetLowering::C_Memory ||
         OpInfo.ConstraintType == TargetLowering::C_Other) {
       if (OpInfo.Type == InlineAsm::isInput)
         Flags |= InlineAsm::Extra_MayLoad;
       else if (OpInfo.Type == InlineAsm::isOutput)
         Flags |= InlineAsm::Extra_MayStore;
       else if (OpInfo.Type == InlineAsm::isClobber)
         Flags |= (InlineAsm::Extra_MayLoad | InlineAsm::Extra_MayStore);
     }
   }
 
   unsigned get() const { return Flags; }
 };
 
 } // end anonymous namespace
 
 /// visitInlineAsm - Handle a call to an InlineAsm object.
 void SelectionDAGBuilder::visitInlineAsm(ImmutableCallSite CS) {
+  LLVMContext &Ctx = *DAG.getContext();
   const InlineAsm *IA = cast<InlineAsm>(CS.getCalledValue());
 
   /// ConstraintOperands - Information about all of the constraints.
   SDISelAsmOperandInfoVector ConstraintOperands;
 
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
   TargetLowering::AsmOperandInfoVector TargetConstraints = TLI.ParseConstraints(
       DAG.getDataLayout(), DAG.getSubtarget().getRegisterInfo(), CS);
if (OpInfo.Type == InlineAsm::isInput ||
       } else if (const auto *BB = dyn_cast<BasicBlock>(OpInfo.CallOperandVal)) {
         OpInfo.CallOperand = DAG.getBasicBlock(FuncInfo.MBBMap[BB]);
       } else {
         OpInfo.CallOperand = getValue(OpInfo.CallOperandVal);
       }
 
       OpInfo.ConstraintVT =
           OpInfo
-              .getCallOperandValEVT(*DAG.getContext(), TLI, DAG.getDataLayout())
+              .getCallOperandValEVT(Ctx, TLI, DAG.getDataLayout())
               .getSimpleVT();
     } else if (OpInfo.Type == InlineAsm::isOutput && !OpInfo.isIndirect) {
       // The return value of the call is this value.  As such, there is no
       // corresponding argument.
       assert(!CS.getType()->isVoidTy() && "Bad inline asm!");
       if (StructType *STy = dyn_cast<StructType>(CS.getType())) {
         OpInfo.ConstraintVT = TLI.getSimpleValueType(
             DAG.getDataLayout(), STy->getElementType(ResNo));
if (!HasSideEffect)
       HasSideEffect = OpInfo.hasMemory(TLI);
 
     // Determine if this InlineAsm MayLoad or MayStore based on the constraints.
     // FIXME: Could we compute this on OpInfo rather than T?
 
     // Compute the constraint code and ConstraintType to use.
     TLI.ComputeConstraintToUse(T, SDValue());
 
+    if (T.ConstraintCode == "n" && !isa<ConstantSDNode>(OpInfo.CallOperand))
+      // We've delayed emitting a diagnostic for the "n" constraint because
+      // inlining could cause an integer showing up.
+      return emitInlineAsmError(CS, "constraint 'n' expects an integer "
+                                "constant expression");
+
     ExtraInfo.update(T);
   }
 
   // We won't need to flush pending loads if this asm doesn't touch
   // memory and is nonvolatile.
   SDValue Flag, Chain = (HasSideEffect) ? getRoot() : DAG.getRoot();
 
   // Second pass over the constraints: compute which constraint option to use.

test/CodeGen/X86/inline-asm-bad-constraint-n.ll

 ; RUN: not llc -mtriple=i686-- -no-integrated-as < %s 2>&1 | FileCheck %s
 
 @x = global i32 0, align 4
 
-;CHECK: error: invalid operand for inline asm constraint 'n'
+; CHECK: error: constraint 'n' expects an integer constant expression
 define void @foo() {
   %a = getelementptr i32, i32* @x, i32 1
   call void asm sideeffect "foo $0", "n"(i32* %a) nounwind
   ret void
 }