[Hexagon] Generate trap/undef if misaligned access is detected

[kparzysz]

Summary

Follow-up to D50405: replace the fatal error with a remark and replace the offending instruction with a trap.

[efriedma]

Converting the memory operation to an explicit trap seems overly complicated, as opposed to just fixing whatever isel pattern is assuming the immediate is divisible by 4. But I don't know the Hexagon backend that well.

Could you use the optimization remark emission infrastructure for this, instead of defining your own DiagnosticInfo? SelectionDAG::getORE() returns an optimization remark emitter. Granted, that wouldn't do exactly the same thing, since optimization remarks are only emitted when requested.

[kparzysz]

Fix the pattern to generate a load with the misaligned address? That already happens. The problem is that we have passes that make changes based on the assumption that the instructions are valid. We make no effort to gracefully handle obviously invalid code, and if we detect it, it is usually in an assert. I think that the trap is a compromise where it won't trigger any further problems during compilation and will still fail for the user.

I'd really like to print the message without users specifically requesting it. The reason being that there is no intuitive connection between a runtime fault and the optimization remark emitter. The AMDGPU isel lowering already uses the same diagnostic infrastructure (except that they don't define their own kind). Do you have a strong preference to use the ORE instead?

[efriedma]

Fix the pattern to generate a load with the misaligned address? That already happens. The problem is that we have passes that make changes based on the assumption that the instructions are valid.

I mean, fix the patterns so that you materialize the immediate into a register. That should be enough unless some later pass tries to fold an immediate followed by a load... in which case that pass is still broken because some cases of immediate+load won't show up until after isel (for example, tail-dup can eliminate a PHI node).

[kparzysz]

I am much more inclined to detect and handle invalid code early than to teach the whole backend to propagate it to the end. If a user decides to use an external assembler, code like that can trigger an error, while using internal assembler will simply hide it.

[efriedma]

There's nothing wrong with simplifying code that you've proven has undefined behavior; we do that all over the place in LLVM. We already have code that specifically detects invalid loads/stores in instcombine (although it currently doesn't try to check alignment). That said, the Hexagon backend still has to be prepared for the possibility that some later pass will introduce the sequence "r0 = ##74565; r0 = memw(r0+#0)", because each of those instructions is independently valid. So this is just papering over the underlying compiler crash.

If a user decides to use an external assembler, code like that can trigger an error

If the Hexagon backend is generating assembly that isn't valid according to the assembler, that has to be fixed in the asmprinter, or some very late MIR pass; isel is too early to catch all cases.

[efriedma]

Example of what I mean; build with clang --target=hexagon -S -O2:

int a(int f(void)) { int *p = (int*)0x123450; if (f()) { f(); p = (int*)0x123456; } return *p; }

Produces:

{
        r0 = ##1193046
        r17:16 = memd(r29+#0)
}                               // 8-byte Folded Reload
{
        r0 = memw(r0+#0)
        dealloc_return
}

[kparzysz]

I understand your point, which I believe is that the user can always sneak in some invalid code using valid instructions, which then some pass will transform into an invalid instruction. This is actually motivated by such a case (an assert cause by user program). The problem is that the assert also helped find compiler bugs, so in that sense it served its purpose. This patch is meant to address the problem in the user code before it gets to that pass, or essentially to filter out user problems before they can no longer be differentiated from internal compiler errors. It isn't intended to catch all possible forms of invalid input, but I believe it covers the only way that the user's program can trigger that particular assertion.

The point about internal vs. external assembler was that if we accept and propagate invalid instructions down the transformation sequence, the integrated assembler will eventually need to generate some code (since it shouldn't report errors). When generating the textual assembly, a wrong instruction can easily be printed, but then the external assembler would pick it up. Patching up invalid instructions in the assembly printer is a bit much, besides, this was really a hypothetical situation (for the purpose of illustrating my point).

[kparzysz]

Btw, I didn't see your update when writing my reply, but it confirms what I thought.

[efriedma]

It might be worth discussing on llvmdev the question of whether it makes sense to add a flag to clang to control general undefined-behavior diagnostics, as a sort of best-effort thing when the optimizer finds it. We have lib/Analysis/Lint.cpp, but there isn't any clang flag to turn it on. Undefined behavior diagnostics are not something that would ever be reliable in the sense of consistently printing the same diagnostics across compiler versions/optimization levels, and it would always have false positives, so I don't think we would ever turn it on by default. But it could be useful to help users figure out "why did my code disappear" when the compiler generates something unexpected.

[kparzysz]

Rebased the patch.

This patch happens to fix https://bugs.llvm.org/show_bug.cgi?id=50838.

Whether the remark should be emitted or not may still need to be decided.

[bcain]

llvm/lib/Target/Hexagon/HexagonISelLowering.cpp

1978 ↗

(On Diff #354522)

Do the tests cover all of the new paths defined here?

[kparzysz]

llvm/lib/Target/Hexagon/HexagonISelLowering.cpp

1978 ↗

(On Diff #354522)

Turns out it's impossible to get an indexed load/store to an immediate address, since DAG combiner will simply generate the updated address directly, instead of relying on pre- or post-increment.

[kparzysz]

Removed handling of indexes loads/stores.

[kparzysz]

Ping.

[bcain]

LGTM

lib/Target/Hexagon/HexagonISelLowering.h

public:
 
     AtomicExpansionKind
     shouldExpandAtomicRMWInIR(AtomicRMWInst *AI) const override {
       return AtomicExpansionKind::LLSC;
     }
 
   private:
     void initializeHVXLowering();
-    void validateConstPtrAlignment(SDValue Ptr, const SDLoc &dl,
-                                   unsigned NeedAlign) const;
+    bool validateConstPtrAlignment(SDValue Ptr, const SDLoc &dl,
+                                   unsigned NeedAlign, SelectionDAG &DAG) const;
+    SDValue replaceMemWithUndef(SDValue Op, SelectionDAG &DAG) const;
 
     std::pair<SDValue,int> getBaseAndOffset(SDValue Addr) const;
 
     bool getBuildVectorConstInts(ArrayRef<SDValue> Values, MVT VecTy,
                                  SelectionDAG &DAG,
                                  MutableArrayRef<ConstantInt*> Consts) const;
     SDValue buildVector32(ArrayRef<SDValue> Elem, const SDLoc &dl, MVT VecTy,
                           SelectionDAG &DAG) const;

lib/Target/Hexagon/HexagonISelLowering.cpp

 #include "llvm/CodeGen/RuntimeLibcalls.h"
 #include "llvm/CodeGen/SelectionDAG.h"
 #include "llvm/CodeGen/TargetCallingConv.h"
 #include "llvm/CodeGen/ValueTypes.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/CallingConv.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DerivedTypes.h"
+#include "llvm/IR/DiagnosticInfo.h"
+#include "llvm/IR/DiagnosticPrinter.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/IR/InlineAsm.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Type.h"
const char* HexagonTargetLowering::getTargetNodeName(unsigned Opcode) const {
   case HexagonISD::TYPECAST:      return "HexagonISD::TYPECAST";
   case HexagonISD::VALIGN:        return "HexagonISD::VALIGN";
   case HexagonISD::VALIGNADDR:    return "HexagonISD::VALIGNADDR";
   case HexagonISD::OP_END:        break;
   }
   return nullptr;
 }
 
-void
+bool
 HexagonTargetLowering::validateConstPtrAlignment(SDValue Ptr, const SDLoc &dl,
-      unsigned NeedAlign) const {
+      unsigned NeedAlign, SelectionDAG &DAG) const {
   auto *CA = dyn_cast<ConstantSDNode>(Ptr);
   if (!CA)
-    return;
+    return true;
   unsigned Addr = CA->getZExtValue();
   unsigned HaveAlign = Addr != 0 ? 1u << countTrailingZeros(Addr) : NeedAlign;
-  if (HaveAlign < NeedAlign) {
-    std::string ErrMsg;
-    raw_string_ostream O(ErrMsg);
-    O << "Misaligned constant address: " << format_hex(Addr, 10)
-      << " has alignment " << HaveAlign
-      << ", but the memory access requires " << NeedAlign;
-    if (DebugLoc DL = dl.getDebugLoc())
-      DL.print(O << ", at ");
-    report_fatal_error(O.str());
-  }
+  if (HaveAlign >= NeedAlign)
+    return true;
+
+  static int DK_MisalignedTrap = llvm::getNextAvailablePluginDiagnosticKind();
+
+  struct DiagnosticInfoMisalignedTrap : public DiagnosticInfo {
+    DiagnosticInfoMisalignedTrap(StringRef M)
+      : DiagnosticInfo(DK_MisalignedTrap, DS_Remark), Msg(M) {}
+    void print(DiagnosticPrinter &DP) const override {
+      DP << Msg;
+    }
+    static bool classof(const DiagnosticInfo *DI) {
+      return DI->getKind() == DK_MisalignedTrap;
+    }
+    StringRef Msg;
+  };
+
+  O << ". The instruction has been replaced with a trap.";
+
+  DAG.getContext()->diagnose(DiagnosticInfoMisalignedTrap(O.str()));
+  return false;
+
+SDValue
+HexagonTargetLowering::replaceMemWithUndef(SDValue Op, SelectionDAG &DAG)
+      const {
+  const SDLoc &dl(Op);
+  auto *LS = cast<LSBaseSDNode>(Op.getNode());
+  SDValue Chain = LS->getChain();
+  SDValue Addr = LS->getBasePtr();
+  SDValue Off = LS->getOffset();
+
+  SDValue Trap = DAG.getNode(ISD::TRAP, dl, MVT::Other, Chain);
+  unsigned Opc = LS->getOpcode();
+
+  if (LS->isIndexed()) {
+    SDValue Inc = DAG.getNode(ISD::ADD, dl, ty(Addr), Addr, Off);
+    if (Opc == ISD::LOAD)
+      return DAG.getMergeValues({DAG.getUNDEF(ty(Op)), Inc, Trap}, dl);
+    return DAG.getMergeValues({Inc, Trap}, dl);
+  }
+
+  if (Opc == ISD::LOAD)
+    return DAG.getMergeValues({DAG.getUNDEF(ty(Op)), Trap}, dl);
+  return Trap;
 }
 
 // Bit-reverse Load Intrinsic: Check if the instruction is a bit reverse load
 // intrinsic.
 static bool isBrevLdIntrinsic(const Value *Inst) {
   unsigned ID = cast<IntrinsicInst>(Inst)->getIntrinsicID();
   return (ID == Intrinsic::hexagon_L2_loadrd_pbr ||
           ID == Intrinsic::hexagon_L2_loadri_pbr ||
HexagonTargetLowering::allowTruncateForTailCall(Type *Ty1, Type *Ty2) const {
   // liable to disallow tail calls involving i64 -> iN truncation in many cases.
   return Ty1->getPrimitiveSizeInBits() <= 32;
 }
 
 SDValue
 HexagonTargetLowering::LowerLoad(SDValue Op, SelectionDAG &DAG) const {
   LoadSDNode *LN = cast<LoadSDNode>(Op.getNode());
   unsigned ClaimAlign = LN->getAlignment();
-  validateConstPtrAlignment(LN->getBasePtr(), SDLoc(Op), ClaimAlign);
+  if (!validateConstPtrAlignment(LN->getBasePtr(), SDLoc(Op), ClaimAlign, DAG))
+    return replaceMemWithUndef(Op, DAG);
   // Call LowerUnalignedLoad for all loads, it recognizes loads that
   // don't need extra aligning.
   return LowerUnalignedLoad(Op, DAG);
 }
 
 SDValue
 HexagonTargetLowering::LowerStore(SDValue Op, SelectionDAG &DAG) const {
   StoreSDNode *SN = cast<StoreSDNode>(Op.getNode());
   unsigned ClaimAlign = SN->getAlignment();
-  SDValue Ptr = SN->getBasePtr();
-  const SDLoc &dl(Op);
-  validateConstPtrAlignment(Ptr, dl, ClaimAlign);
+  if (!validateConstPtrAlignment(SN->getBasePtr(), SDLoc(Op), ClaimAlign, DAG))
+    return replaceMemWithUndef(Op, DAG);
 
   MVT StoreTy = SN->getMemoryVT().getSimpleVT();
   unsigned NeedAlign = Subtarget.getTypeAlignment(StoreTy);
   if (ClaimAlign < NeedAlign)
     return expandUnalignedStore(SN, DAG);
   return Op;
 }
 

test/CodeGen/Hexagon/misaligned-const-load.ll

-; RUN: not llc -march=hexagon < %s 2>&1 | FileCheck %s
+; RUN: llc -march=hexagon < %s 2>&1 | FileCheck %s
 
 ; Check that the misaligned load is diagnosed.
-; CHECK: LLVM ERROR: Misaligned constant address: 0x00012345 has alignment 1, but the memory access requires 4, at misaligned-const-load.c:2:10
+; CHECK: remark: Misaligned constant address: 0x00012345 has alignment 1, but the memory access requires 4, at misaligned-const-load.c:2:10. The instruction has been replaced with a trap.
 
 target triple = "hexagon"
 
 define i32 @bad_load() #0 !dbg !10 {
 entry:
   %0 = load i32, i32* inttoptr (i32 74565 to i32*), align 4, !dbg !13, !tbaa !14
   ret i32 %0, !dbg !18
 }

test/CodeGen/Hexagon/misaligned-const-store.ll

-; RUN: not llc -march=hexagon < %s 2>&1 | FileCheck %s
+; RUN: llc -march=hexagon < %s 2>&1 | FileCheck %s
 
 ; Check that the misaligned store is diagnosed.
-; CHECK: LLVM ERROR: Misaligned constant address: 0x00012345 has alignment 1, but the memory access requires 4, at misaligned-const-store.c:2:10
+; CHECK: remark: Misaligned constant address: 0x00012345 has alignment 1, but the memory access requires 4, at misaligned-const-store.c:2:10. The instruction has been replaced with a trap.
 
 target triple = "hexagon"
 
 define void @bad_store(i32 %a0) #0 !dbg !10 {
 entry:
   store i32 %a0, i32* inttoptr (i32 74565 to i32*), align 4, !dbg !13, !tbaa !14
   ret void, !dbg !18
 }