[PATCH] D69099: [TargetLowering][DAGCombine][MSP430] Shift Amount Threshold in DAGCombine (Baseline Tests)

[Joan LLuch via Phabricator via llvm-commits]

joanlluch marked 2 inline comments as done.
joanlluch added inline comments.


================
Comment at: llvm/test/CodeGen/MSP430/shift-amount-threshold.ll:3-4
+; RUN: llc < %s | FileCheck %s
+target datalayout = "e-m:e-p:16:16-i32:16-i64:16-f32:16-f64:16-a:8-n8:16-S16"
+target triple = "msp430"
+
----------------
spatel wrote:
> I'm not sure if there's a difference, but I usually specify the triple in the RUN line, and I don't explicitly set a datalayout.
I think there's no difference. I just followed the pattern found in other tests of the same target such as the shifts.ll. Also, the form that I used is what clang generates, so I thought it would be more representative. But I can replace it if considered not suitable.


================
Comment at: llvm/test/CodeGen/MSP430/shift-amount-threshold.ll:6
+
+define dso_local i16 @testSimplifySetCC_0(i16 %a) {
+; CHECK-LABEL: testSimplifySetCC_0:
----------------
spatel wrote:
> Here and below: remove unnecessary "dso_local"
I'm unsure about what dsl_local means, this was generated by clang, but I'll remove it if not necessary.


================
Comment at: llvm/test/CodeGen/MSP430/shift-amount-threshold.ll:160
+
+define dso_local i16 @testShiftAnd_4(i16 %a, i16 %b) {
+; CHECK-LABEL: testShiftAnd_4:
----------------
spatel wrote:
> Is it intentional that all tests use the i16 type? Would it be beneficial/increase test coverage to include other common types like i32/i64?
Yes, the i16 type is intentional. The target does not natively support larger types, so larger types are expanded into operations with register pairs requiring additional transformations, or are custom lowered, which confuse the purposes of this patch. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69099