[hwasan] Prevent reordering of tag checks.

[fmayer]

Summary

They were previously unconstrained, which allowed them to be reordered
before the shadow memory write.

[fmayer]

commit message change

[fmayer]

I ran 50 iterations of pdfium_test with baseline vs. this change and could see no statistically significant change (p-value is 88%): https://colab.research.google.com/drive/1BBOT0BYGnvS9bvbryMNyAsVBbVM30gx5#scrollTo=cfis4f2vI305

[eugenis]

Is mayStore=1 necessary? It does not make any sense, these instruction do not write to memory.

[fmayer]

only mayload

[fmayer]

In D107901#2940403, @eugenis wrote:

Is mayStore=1 necessary? It does not make any sense, these instruction do not write to memory.

That also works. I don't think it's necessary to re-run the benchmarks as it's not expected that things are worse like this.

[eugenis]

It would be nice to have a test for this, maybe something like test/CodeGen/AArch64/irg-nomem.mir (but the opposite - you'd want store merging to not happen).

Another weird thing: even without this patch I see

{ 561,        2,      0,      0,      0,      0|(1ULL<<MCID::Pseudo)|(1ULL<<MCID::MayLoad)|(1ULL<<MCID::MayStore), 0x0ULL, ImplicitList4, ImplicitList5, OperandInfo65 },  // Inst #561 = HWASAN_CHECK_MEMACCESS
{ 562,        2,      0,      0,      0,      0|(1ULL<<MCID::Pseudo)|(1ULL<<MCID::MayLoad)|(1ULL<<MCID::MayStore), 0x0ULL, ImplicitList6, ImplicitList5, OperandInfo65 },  // Inst #562 = HWASAN_CHECK_MEMACCESS_SHORTGRANULES

in lib/Target/AArch64/AArch64GenInstrInfo.inc, i.e. these instructions are already mayLoad | mayStore.

[eugenis]

I find this program useful to test aliasing behavior:

void zzz(long *x) {
  x[0] = 42;
  x[1] = 42;
}

With -fno-slp-vectorize it generates store - check - store sequence where two stores can be merged into store-pair (see AArch64LoadStoreOptimizer) if hwasan check is not mayLoadOrStore.

[fmayer]

In D107901#2942065, @eugenis wrote:

Another weird thing: even without this patch I see

{ 561,        2,      0,      0,      0,      0|(1ULL<<MCID::Pseudo)|(1ULL<<MCID::MayLoad)|(1ULL<<MCID::MayStore), 0x0ULL, ImplicitList4, ImplicitList5, OperandInfo65 },  // Inst #561 = HWASAN_CHECK_MEMACCESS
{ 562,        2,      0,      0,      0,      0|(1ULL<<MCID::Pseudo)|(1ULL<<MCID::MayLoad)|(1ULL<<MCID::MayStore), 0x0ULL, ImplicitList6, ImplicitList5, OperandInfo65 },  // Inst #562 = HWASAN_CHECK_MEMACCESS_SHORTGRANULES

in lib/Target/AArch64/AArch64GenInstrInfo.inc, i.e. these instructions are already mayLoad | mayStore.

I tried to revert this part and it still solves the issues. I would still prefer to leave it as is to have it more explicit.

[fmayer]

revert unnecessary change

[fmayer]

add test

[fmayer]

add missing newline

[fmayer]

I verified this test fails at baseline and passes with this change.

[eugenis]

LGTM

Please check that the test works in release-without-assertions build. It tends to loose names of temporaries like "%x.hwasan".

[fmayer]

In D107901#2947723, @eugenis wrote:

LGTM

Please check that the test works in release-without-assertions build. It tends to loose names of temporaries like "%x.hwasan".

passes with -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=0

[aeubanks]

llvm/test/Instrumentation/HWAddressSanitizer/memaccess-clobber.ll

3 ↗

(On Diff #366843)

this is the only user of print-memdeps which is a legacy PM-specific pass. -analyze is also a legacy PM-specific feature.
is there a better way of testing this? memdep is deprecated anyway

[fmayer]

llvm/test/Instrumentation/HWAddressSanitizer/memaccess-clobber.ll

3 ↗

(On Diff #366843)

I don't know of any more direct way to test this. Is there in the new PM that you know about that can give information about memory dependencies / what can't be reordered?

[aeubanks]

llvm/test/Instrumentation/HWAddressSanitizer/memaccess-clobber.ll

3 ↗

(On Diff #366843)

D119393

llvm/include/llvm/IR/Intrinsics.td

 // callees. This needs to be a musttail call.
 def int_icall_branch_funnel : DefaultAttrsIntrinsic<[], [llvm_vararg_ty], []>;
 
 def int_load_relative: DefaultAttrsIntrinsic<[llvm_ptr_ty], [llvm_ptr_ty, llvm_anyint_ty],
                                  [IntrReadMem, IntrArgMemOnly]>;
 
 def int_hwasan_check_memaccess :
   Intrinsic<[], [llvm_ptr_ty, llvm_ptr_ty, llvm_i32_ty],
-            [IntrInaccessibleMemOnly, ImmArg<ArgIndex<2>>]>;
+            [ImmArg<ArgIndex<2>>]>;
 def int_hwasan_check_memaccess_shortgranules :
   Intrinsic<[], [llvm_ptr_ty, llvm_ptr_ty, llvm_i32_ty],
-            [IntrInaccessibleMemOnly, ImmArg<ArgIndex<2>>]>;
+            [ImmArg<ArgIndex<2>>]>;
 
 // Xray intrinsics
 //===----------------------------------------------------------------------===//
 // Custom event logging for x-ray.
 // Takes a pointer to a string and the length of the string.
 def int_xray_customevent : Intrinsic<[], [llvm_ptr_ty, llvm_i32_ty],
                                      [IntrWriteMem, NoCapture<ArgIndex<0>>,
                                       ReadOnly<ArgIndex<0>>]>;

llvm/lib/Target/AArch64/AArch64InstrInfo.td

 def : Pat<(AArch64mrs imm:$id),
           (MRS imm:$id)>;
 
 // The thread pointer (on Linux, at least, where this has been implemented) is
 // TPIDR_EL0.
 def MOVbaseTLS : Pseudo<(outs GPR64:$dst), (ins),
                        [(set GPR64:$dst, AArch64threadpointer)]>, Sched<[WriteSys]>;
 
-let Uses = [ X9 ], Defs = [ X16, X17, LR, NZCV ] in {
+let Uses = [ X9 ], Defs = [ X16, X17, LR, NZCV ], mayLoad = 1, mayStore = 1 in {
 def HWASAN_CHECK_MEMACCESS : Pseudo<
   (outs), (ins GPR64noip:$ptr, i32imm:$accessinfo),
   [(int_hwasan_check_memaccess X9, GPR64noip:$ptr, (i32 timm:$accessinfo))]>,
   Sched<[]>;
 }
 
-let Uses = [ X20 ], Defs = [ X16, X17, LR, NZCV ] in {
+let Uses = [ X20 ], Defs = [ X16, X17, LR, NZCV ], mayLoad = 1, mayStore = 1 in {
 def HWASAN_CHECK_MEMACCESS_SHORTGRANULES : Pseudo<
   (outs), (ins GPR64noip:$ptr, i32imm:$accessinfo),
   [(int_hwasan_check_memaccess_shortgranules X20, GPR64noip:$ptr, (i32 timm:$accessinfo))]>,
   Sched<[]>;
 }
 
 // The cycle counter PMC register is PMCCNTR_EL0.
 let Predicates = [HasPerfMon] in