dblaikie added a comment.
In D85408#2262134 <https://reviews.llvm.org/D85408#2262134>, @rahmanl wrote:
> @efriedma Would you please chime in specially with respect to @MaskRay 's
> comment about the clang test involving assembly?
I'll chime in, perhaps @efriedma will/can too: This patch has no changes to
Clang's code, so it should have no changes to Clang's tests, generally speaking.
Whatever codepath/behavior is being tested by that Clang test change should be
testable (& generally only tested) via LLVM's tools/tests, like llc, I would
expect? (I guess that's what the basic-block-sections-labels.ll test is doing?
Making the Clang test update redundant)
The only reason I'd expect to see the assembly tested in Clang is if the flag
that enables this feature is an MCOptions (or other similar API level feature)
member, rather than part of LLVM IR proper. In that case the only way to test
that the flag has any effect (since we can't observe the MCOptions struct state
directly in a test) is to test for the MCOptions effect on LLVM's output.
Depending on the complexity, sometimes such tests are just skipped entirely
(leaving the setting of the MCOption untested) - I think I did that for, for
example, DWARF type units. Other times, an end-to-end test is used to ensure
the flag is working. But that's all the test should do: test the flag is
working, with some very basic/rudimentary property of the feature being enabled
(so pretty much /any/ change to the way the feature works will still pass the
test, but if the feature were turned off entirely (the MCOptions flag stopped
being set correctly), the test would fail). Not test all the functionality of
the feature that the flag enables - all that testing should be down in LLVM.
================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1058
+ // Emit BB Information for each basic block in the funciton.
+ for (const auto &MBB : MF) {
+ const MCSymbol *MBBSymbol =
----------------
MaskRay wrote:
> auto -> MachineBasicBlock
FWIW, auto seems pretty fine to me here - looking only at for loops over MBB, a
rough grep/count seems like there's certainly a lot of both (194 (non-auto) to
168 (auto)) and I think it qualifies as "obvious" for LLVM backend code that an
MBB variable would be of the type MachineBasicBlock. (bonus points for
obviousness that MF is a MachineFunction)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D85408/new/
https://reviews.llvm.org/D85408
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
