[WebAssembly] Print error message for llvm.clear_cache intrinsic

[aheejin]

Summary

Wasm does not currently support llvm.clear_cache intrinsic, and this
prints a proper error message instead of segfault.

[sunfish]

Any code that thinks it needs to "clear the cache" is very likely doing something which won't actually work on wasm. Would it be reasonable to issue a report_fatal_error here?

[sbc100]

This looks like we we doing the exact same thing that x86 is. So it would make sense to me that whatever we end up doing we should continue to match x86. Perhaps landing this as-is and then opening a bug to turn this into an unreachable (for both x86 and wasm) is one option?

[aheejin]

Yes it is what x86 does too. And it looks like they do it on purpose (= it's not a bug). From the [[ http://llvm.org/docs/LangRef.html#llvm-clear-cache-intrinsic | llvm.clear_cache ]] intrinsic semantics description:
"On platforms with coherent instruction and data caches (e.g. x86), this intrinsic is a nop. On platforms with non-coherent instruction and data cache (e.g. ARM, MIPS), the intrinsic is lowered either to appropriate instructions or a system call, if cache flushing requires special privileges. The default behavior is to emit a call to __clear_cache from the run time library."

So it sounds like only platforms with non-coherent instruction and data caches should do something. Wasm does not have concepts of instruction or data cache, but because it is designed to be more of a virtual machine that does not expose incoherences of hardwares, I thought it might make sense to do nothing like x86 as if we have coherent caches. But I don't have strong opinions on either side.

[aheejin]

In D64322#1573411, @sunfish wrote:

Any code that thinks it needs to "clear the cache" is very likely doing something which won't actually work on wasm. Would it be reasonable to issue a report_fatal_error here?

I'm not sure it would be easy, because the only hook we have for each target is that getClearCacheBuiltinName function.

[sunfish]

The actual compiler crash here came from the fact that we don't have an entry for llvm.clear_cache in the builtin function signature table. Suppose we added a signature for it. Then, we'd get LLVM's and compiler-rt's default behavior, which would be to call __clear_cache, and ultimately just abort at runtime.

So, what if we do report_fatal_error("llvm.clear_cache is not supported on wasm"); from within getClearCacheBuiltinName? That's friendlier than aborting at runtime at least.

I think of wasm as being at the opposite end of the spectrum from x86 in this area. When you write to memory:

• x86: you can execute it immediately, given the right permissions
• arm: you can't execute it until you flush the cache
• wasm: you can't execute it ever

[dschuff]

Another view would be that we accept that this probably doesn't get used in code that would be useful in wasm, but maybe it's in a file that has other useful code, so this would basically be a convenience feature that allows users to have fewer ifdefs in their files (even with the assumption that they aren't going to be calling this code). Someone did, after all, actually run into this problem. Speaking of, maybe we should ask them what their use case is, why they even want to compile this code?

[aheejin]

In D64322#1576662, @dschuff wrote:

Another view would be that we accept that this probably doesn't get used in code that would be useful in wasm, but maybe it's in a file that has other useful code, so this would basically be a convenience feature that allows users to have fewer ifdefs in their files (even with the assumption that they aren't going to be calling this code). Someone did, after all, actually run into this problem. Speaking of, maybe we should ask them what their use case is, why they even want to compile this code?

Don't know, but I guess most likely he was trying to port some code that runs on other platforms...?

[aheejin]

I thought about it, and maybe it would be safer to be explicit that we don't support clearing caches after all. I don't have strong opinions though.

[aheejin]

• Print an error message instead

[dschuff]

This is fine (assuming getClearCacheBuiltinName is only called when clear_cache is itself called in the source), and an improvement over just asserting. I'm curious to hear what the OP says about their use case, we can always discuss changing this in the future.

llvm/trunk/lib/Target/WebAssembly/WebAssemblyISelLowering.h

SDValue LowerFormalArguments(SDValue Chain, CallingConv::ID CallConv,
                                bool IsVarArg,
                                const SmallVectorImpl<ISD::InputArg> &Ins,
                                const SDLoc &DL, SelectionDAG &DAG,
                                SmallVectorImpl<SDValue> &InVals) const override;
 
   void ReplaceNodeResults(SDNode *N, SmallVectorImpl<SDValue> &Results,
                           SelectionDAG &DAG) const override;
 
+  const char *getClearCacheBuiltinName() const override {
+    report_fatal_error("llvm.clear_cache is not supported on wasm");
+  }
+
   // Custom lowering hooks.
   SDValue LowerOperation(SDValue Op, SelectionDAG &DAG) const override;
   SDValue LowerFrameIndex(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerRETURNADDR(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerFRAMEADDR(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerGlobalAddress(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerExternalSymbol(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerBR_JT(SDValue Op, SelectionDAG &DAG) const;

llvm/trunk/test/CodeGen/WebAssembly/clear-cache.ll

+; RUN: not llc < %s -asm-verbose=false 2>&1 | FileCheck %s
+
+target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
+target triple = "wasm32-unknown-unknown"
+
+; CHECK: LLVM ERROR: llvm.clear_cache is not supported on wasm
+define void @clear_cache(i8* %begin, i8* %end) {
+entry:
+  call void @llvm.clear_cache(i8* %begin, i8* %end)
+  ret void
+}
+
+declare void @llvm.clear_cache(i8*, i8*)