[PATCH] D147348: [MergedLoadStoreMotion] Merge stores with conflicting value types

[Nikita Popov via Phabricator via llvm-commits]

nikic requested changes to this revision.
nikic added a comment.
This revision now requires changes to proceed.

The logic here looks fine to me now. I'm not a bit fan of the change to isSameOperationAs(), please see the inline suggestion.



================
Comment at: llvm/lib/IR/Instruction.cpp:589
   bool UseScalarTypes  = flags & CompareUsingScalarTypes;
+  bool IgnoreType = flags & CompareIgnoringType;
 
----------------
With CompareIgnoringType this basically becomes the same as just haveSameSpecialState(). I'd suggest exposing that as a public method rather than adding this flag.


================
Comment at: llvm/test/Transforms/MergedLoadStoreMotion/st_sink_conflict_type.ll:3
+; RUN: opt -passes=mldst-motion -S < %s | FileCheck %s
+target datalayout = "e-m:o-i64:64-i128:128-n32:64-S128"
+
----------------
Probably not needed.


================
Comment at: llvm/test/Transforms/MergedLoadStoreMotion/st_sink_conflict_type.ll:5
+
+define internal fastcc void @sink_conflict(ptr %this.64.val, half %val1, i16 %val2, i32 %val3) unnamed_addr align 2 {
+; CHECK-LABEL: define internal fastcc void @sink_conflict
----------------
fastcc/unnamed_addr not needed


================
Comment at: llvm/test/Transforms/MergedLoadStoreMotion/st_sink_conflict_type.ll:31
+  store half %val1, ptr %this.64.val, align 2
+  %add.ptr.i.i.i.i = getelementptr inbounds i8, ptr %this.64.val, i64 16
+  store i16 %val2, ptr %add.ptr.i.i.i.i, align 2
----------------
Drop the `.i.i.i.i` suffixes here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147348/new/

https://reviews.llvm.org/D147348